From 26d8126497fac54aace06efa2bb5ec5570b01c94 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 11 Sep 2020 14:37:31 +1000 Subject: [PATCH] More tests fixes up a few things with external login service, adds notes changes column to ntext --- .../Persistence/Dtos/ExternalLoginDto.cs | 4 +- .../Factories/ExternalLoginFactory.cs | 18 ++- .../Implement/ExternalLoginRepository.cs | 62 +++++---- .../Security/BackOfficeUserStore.cs | 14 ++- .../Services/ExternalLoginServiceTests.cs | 118 ++++++++++++++++++ .../Editors/BackOfficeController.cs | 8 +- 6 files changed, 180 insertions(+), 44 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Dtos/ExternalLoginDto.cs b/src/Umbraco.Core/Persistence/Dtos/ExternalLoginDto.cs index c98e6c1787..d58d23a272 100644 --- a/src/Umbraco.Core/Persistence/Dtos/ExternalLoginDto.cs +++ b/src/Umbraco.Core/Persistence/Dtos/ExternalLoginDto.cs @@ -17,6 +17,8 @@ namespace Umbraco.Core.Persistence.Dtos [Column("userId")] public int UserId { get; set; } + // TODO: There should be an index on both LoginProvider and ProviderKey + [Column("loginProvider")] [Length(4000)] [NullSetting(NullSetting = NullSettings.NotNull)] @@ -35,8 +37,8 @@ namespace Umbraco.Core.Persistence.Dtos /// Used to store any arbitrary data for the user and external provider - like user tokens returned from the provider /// [Column("userData")] - [Length(4000)] [NullSetting(NullSetting = NullSettings.Null)] + [SpecialDbType(SpecialDbTypes.NTEXT)] public string UserData { get; set; } } } diff --git a/src/Umbraco.Core/Persistence/Factories/ExternalLoginFactory.cs b/src/Umbraco.Core/Persistence/Factories/ExternalLoginFactory.cs index 2dc2d25604..6c1af68acd 100644 --- a/src/Umbraco.Core/Persistence/Factories/ExternalLoginFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/ExternalLoginFactory.cs @@ -1,4 +1,5 @@ -using Umbraco.Core.Models.Identity; +using System; +using Umbraco.Core.Models.Identity; using Umbraco.Core.Persistence.Dtos; namespace Umbraco.Core.Persistence.Factories @@ -32,5 +33,20 @@ namespace Umbraco.Core.Persistence.Factories return dto; } + + public static ExternalLoginDto BuildDto(int userId, IExternalLogin entity, int? id = null) + { + var dto = new ExternalLoginDto + { + Id = id ?? default, + UserId = userId, + LoginProvider = entity.LoginProvider, + ProviderKey = entity.ProviderKey, + UserData = entity.UserData, + CreateDate = DateTime.Now + }; + + return dto; + } } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ExternalLoginRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ExternalLoginRepository.cs index 5055145af6..90a698f999 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ExternalLoginRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ExternalLoginRepository.cs @@ -33,51 +33,49 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .Where(x => x.UserId == userId) .ForUpdate(); + // deduplicate the logins + logins = logins.DistinctBy(x => x.ProviderKey + x.LoginProvider).ToList(); + var toUpdate = new Dictionary(); var toDelete = new List(); var toInsert = new List(logins); - var existingLogins = Database.Fetch(sql); + 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) { - var found = logins.FirstOrDefault(x => - x.LoginProvider.Equals(existing.LoginProvider, StringComparison.InvariantCultureIgnoreCase) - && x.ProviderKey.Equals(existing.ProviderKey, StringComparison.InvariantCultureIgnoreCase)); - - if (found != null) + if (!keys.Add((existing.ProviderKey, existing.LoginProvider))) { - 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); + // if it already exists we need to remove this one + toDelete.Add(existing.Id); } else { - toDelete.Add(existing.Id); + 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); + } } } - // do the deletes (does this syntax work?) - Database.DeleteMany().Where(x => toDelete.Contains(x.Id)).Execute(); + // 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(new ExternalLoginDto - { - Id = u.Key, - LoginProvider = u.Value.LoginProvider, - ProviderKey = u.Value.ProviderKey, - UserId = userId, - CreateDate = DateTime.Now - }); - } - // add the inserts - Database.InsertBulk(toInsert.Select(i => new ExternalLoginDto - { - LoginProvider = i.LoginProvider, - ProviderKey = i.ProviderKey, - UserId = userId, - UserData = i.UserData, - CreateDate = DateTime.Now - })); + Database.Update(ExternalLoginFactory.BuildDto(userId, u.Value, u.Key)); + Database.InsertBulk(toInsert.Select(i => ExternalLoginFactory.BuildDto(userId, i))); } public void SaveUserLogins(int memberId, IEnumerable logins) @@ -145,7 +143,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement foreach (var dto in dtos) { - yield return Get(dto.Id); + yield return ExternalLoginFactory.BuildEntity(dto); } } diff --git a/src/Umbraco.Core/Security/BackOfficeUserStore.cs b/src/Umbraco.Core/Security/BackOfficeUserStore.cs index 3a110f9b2c..1ee5646ec5 100644 --- a/src/Umbraco.Core/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Core/Security/BackOfficeUserStore.cs @@ -115,7 +115,7 @@ namespace Umbraco.Core.Security /// /// /// - public async Task UpdateAsync(BackOfficeIdentityUser user) + public Task UpdateAsync(BackOfficeIdentityUser user) { ThrowIfDisposed(); if (user == null) throw new ArgumentNullException(nameof(user)); @@ -126,6 +126,8 @@ namespace Umbraco.Core.Security throw new InvalidOperationException("The user id must be an integer to work with the Umbraco"); } + // TODO: Wrap this in a scope! + var found = _userService.GetUserById(asInt.Result); if (found != null) { @@ -139,10 +141,16 @@ namespace Umbraco.Core.Security if (isLoginsPropertyDirty) { - var logins = await GetLoginsAsync(user); - _externalLoginService.Save(found.Id, logins.Select(x => new ExternalLogin(x.LoginProvider, x.ProviderKey))); + _externalLoginService.Save( + found.Id, + user.Logins.Select(x => new ExternalLogin( + x.LoginProvider, + x.ProviderKey, + (x is IIdentityUserLoginExtended extended) ? extended.UserData : null))); } } + + return Task.CompletedTask; } /// diff --git a/src/Umbraco.Tests/Services/ExternalLoginServiceTests.cs b/src/Umbraco.Tests/Services/ExternalLoginServiceTests.cs index f0f35a9cdf..8a31518ca0 100644 --- a/src/Umbraco.Tests/Services/ExternalLoginServiceTests.cs +++ b/src/Umbraco.Tests/Services/ExternalLoginServiceTests.cs @@ -5,6 +5,7 @@ using Microsoft.AspNet.Identity; using NUnit.Framework; using Umbraco.Core.Models.Identity; using Umbraco.Core.Models.Membership; +using Umbraco.Core.Persistence.Dtos; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.Testing; @@ -15,6 +16,73 @@ namespace Umbraco.Tests.Services [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerFixture)] public class ExternalLoginServiceTests : TestWithDatabaseBase { + [Test] + public void Removes_Existing_Duplicates_On_Save() + { + var user = new User("Test", "test@test.com", "test", "helloworldtest"); + ServiceContext.UserService.Save(user); + + var providerKey = Guid.NewGuid().ToString("N"); + var latest = DateTime.Now.AddDays(-1); + var oldest = DateTime.Now.AddDays(-10); + + using (var scope = ScopeProvider.CreateScope()) + { + // insert duplicates manuall + scope.Database.Insert(new ExternalLoginDto + { + UserId = user.Id, + LoginProvider = "test1", + ProviderKey = providerKey, + CreateDate = latest + }); + scope.Database.Insert(new ExternalLoginDto + { + UserId = user.Id, + LoginProvider = "test1", + ProviderKey = providerKey, + CreateDate = oldest + }); + } + + // try to save 2 other duplicates + var externalLogins = new[] + { + new ExternalLogin("test2", providerKey), + new ExternalLogin("test2", providerKey), + new ExternalLogin("test1", providerKey) + }; + + ServiceContext.ExternalLoginService.Save(user.Id, externalLogins); + + var logins = ServiceContext.ExternalLoginService.GetAll(user.Id).ToList(); + + // duplicates will be removed, keeping the latest entries + Assert.AreEqual(2, logins.Count); + + var test1 = logins.Single(x => x.LoginProvider == "test1"); + Assert.Greater(test1.CreateDate, latest); + } + + [Test] + public void Does_Not_Persist_Duplicates() + { + var user = new User("Test", "test@test.com", "test", "helloworldtest"); + ServiceContext.UserService.Save(user); + + var providerKey = Guid.NewGuid().ToString("N"); + var externalLogins = new[] + { + new ExternalLogin("test1", providerKey), + new ExternalLogin("test1", providerKey) + }; + + ServiceContext.ExternalLoginService.Save(user.Id, externalLogins); + + var logins = ServiceContext.ExternalLoginService.GetAll(user.Id).ToList(); + Assert.AreEqual(1, logins.Count); + } + [Test] public void Single_Create() { @@ -54,6 +122,56 @@ namespace Umbraco.Tests.Services Assert.AreEqual("world", found[0].UserData); } + [Test] + public void Multiple_Update() + { + var user = new User("Test", "test@test.com", "test", "helloworldtest"); + ServiceContext.UserService.Save(user); + + var providerKey1 = Guid.NewGuid().ToString("N"); + var providerKey2 = Guid.NewGuid().ToString("N"); + var extLogins = new[] + { + new ExternalLogin("test1", providerKey1, "hello"), + new ExternalLogin("test2", providerKey2, "world") + }; + ServiceContext.ExternalLoginService.Save(user.Id, extLogins); + + extLogins = new[] + { + new ExternalLogin("test1", providerKey1, "123456"), + new ExternalLogin("test2", providerKey2, "987654") + }; + ServiceContext.ExternalLoginService.Save(user.Id, extLogins); + + var found = ServiceContext.ExternalLoginService.GetAll(user.Id).Cast().OrderBy(x => x.LoginProvider).ToList(); + Assert.AreEqual(2, found.Count); + Assert.AreEqual("123456", found[0].UserData); + Assert.AreEqual("987654", found[1].UserData); + } + + [Test] + public void Can_Find_As_Extended_Type() + { + var user = new User("Test", "test@test.com", "test", "helloworldtest"); + ServiceContext.UserService.Save(user); + + var providerKey1 = Guid.NewGuid().ToString("N"); + var providerKey2 = Guid.NewGuid().ToString("N"); + var extLogins = new[] + { + new ExternalLogin("test1", providerKey1, "hello"), + new ExternalLogin("test2", providerKey2, "world") + }; + ServiceContext.ExternalLoginService.Save(user.Id, extLogins); + + var found = ServiceContext.ExternalLoginService.Find("test2", providerKey2).ToList(); + Assert.AreEqual(1, found.Count); + var asExtended = found.Cast().ToList(); + Assert.AreEqual(1, found.Count); + + } + [Test] public void Add_Logins() { diff --git a/src/Umbraco.Web/Editors/BackOfficeController.cs b/src/Umbraco.Web/Editors/BackOfficeController.cs index 4d0f560c14..c044b6aade 100644 --- a/src/Umbraco.Web/Editors/BackOfficeController.cs +++ b/src/Umbraco.Web/Editors/BackOfficeController.cs @@ -390,7 +390,7 @@ namespace Umbraco.Web.Editors if (response == null) throw new ArgumentNullException("response"); ExternalSignInAutoLinkOptions autoLinkOptions = null; - //Here we can check if the provider associated with the request has been configured to allow + // Here we can check if the provider associated with the request has been configured to allow // new users (auto-linked external accounts). This would never be used with public providers such as // Google, unless you for some reason wanted anybody to be able to access the backend if they have a Google account // .... not likely! @@ -408,12 +408,6 @@ namespace Umbraco.Web.Editors var user = await UserManager.FindAsync(loginInfo.Login); if (user != null) { - // TODO: It might be worth keeping some of the claims associated with the ExternalLoginInfo, in which case we - // wouldn't necessarily sign the user in here with the standard login, instead we'd update the - // UseUmbracoBackOfficeExternalCookieAuthentication extension method to have the correct provider and claims factory, - // ticket format, etc.. to create our back office user including the claims assigned and in this method we'd just ensure - // that the ticket is created and stored and that the user is logged in. - var shouldSignIn = true; if (autoLinkOptions != null && autoLinkOptions.OnExternalLogin != null) {