From 05ccb9e8bbe6285521653c6e4652f5b9cd65c355 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 13:44:02 +0000 Subject: [PATCH] Extended and updated unit tests. Put store methods into try and catch where accessing a service. Added more identity results and try/catches where needed. Updated memberuserstore. Ignore temporary test cached files. --- .gitignore | 1 + .../Security/MemberRoleStore.cs | 294 ++++++++++------ .../Security/MemberUserStore.cs | 320 ++++++++++++------ .../Security/MemberRoleStoreTests.cs | 30 +- 4 files changed, 411 insertions(+), 234 deletions(-) diff --git a/.gitignore b/.gitignore index 0cffac8343..f8a2ce511f 100644 --- a/.gitignore +++ b/.gitignore @@ -201,3 +201,4 @@ src/Umbraco.Tests/TEMP/ /src/Umbraco.Web.UI/config/umbracoSettings.config /src/Umbraco.Web.UI.NetCore/Umbraco/models/* +src/Umbraco.Tests.UnitTests/umbraco/Data/TEMP/ diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 0b91237af9..4477c4870c 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -15,6 +15,11 @@ namespace Umbraco.Cms.Core.Security private readonly IMemberGroupService _memberGroupService; private bool _disposed; + //TODO: Move into custom error describer. + //TODO: How revealing can the error messages be? + private readonly IdentityError _intParseError = new IdentityError { Code = "IdentityIdParseError", Description = "Cannot parse ID to int" }; + private readonly IdentityError _memberGroupNotFoundError = new IdentityError { Code = "IdentityMemberGroupNotFound", Description = "Member group not found" }; + public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber errorDescriber) { _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); @@ -25,99 +30,123 @@ namespace Umbraco.Cms.Core.Security /// Gets or sets the for any error that occurred with the current operation. /// public IdentityErrorDescriber ErrorDescriber { get; set; } - - /// - public Task CreateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - var memberGroup = new MemberGroup - { - Name = role.Name - }; - - _memberGroupService.Save(memberGroup); - - role.Id = memberGroup.Id.ToString(); - - return Task.FromResult(IdentityResult.Success); - } - /// - public Task UpdateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) + public Task CreateAsync(TRole role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) + try { - throw new ArgumentNullException(nameof(role)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); - } - - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - if (MapToMemberGroup(role, memberGroup)) + if (role == null) { - _memberGroupService.Save(memberGroup); + throw new ArgumentNullException(nameof(role)); } - } - else - { - //TODO: throw exception when not found, or return failure? And is this the correct message - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); - } - return Task.FromResult(IdentityResult.Success); + var memberGroup = new MemberGroup + { + Name = role.Name + }; + + _memberGroupService.Save(memberGroup); + + role.Id = memberGroup.Id.ToString(); + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } + } + + + /// + public Task UpdateAsync(TRole role, CancellationToken cancellationToken = default) + { + try + { + cancellationToken.ThrowIfCancellationRequested(); + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + ThrowIfDisposed(); + + if (!int.TryParse(role.Id, out int roleId)) + { + return Task.FromResult(IdentityResult.Failed(_intParseError)); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + if (MapToMemberGroup(role, memberGroup)) + { + _memberGroupService.Save(memberGroup); + } + //TODO: if nothing changed, do we need to report this? + return Task.FromResult(IdentityResult.Success); + } + else + { + //TODO: throw exception when not found, or return failure? + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// - public Task DeleteAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) + public Task DeleteAsync(TRole role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) + try { - throw new ArgumentNullException(nameof(role)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); - } + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - _memberGroupService.Delete(memberGroup); - } - else - { - //TODO: throw exception when not found, or return failure? And is this the correct message - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); - } + if (!int.TryParse(role.Id, out int roleId)) + { + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(_intParseError)); + } - return Task.FromResult(IdentityResult.Success); + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + _memberGroupService.Delete(memberGroup); + } + else + { + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// - public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken) + public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (role == null) { throw new ArgumentNullException(nameof(role)); @@ -126,67 +155,118 @@ namespace Umbraco.Cms.Core.Security return Task.FromResult(role.Id); } - public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken) + /// + public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) + if (role == null) { - return null; + throw new ArgumentNullException(nameof(role)); } - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - - return Task.FromResult(memberGroup?.Name); - } - - public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; - } - - public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; - } - - public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; + return Task.FromResult(role.Name); } /// - public Task FindByIdAsync(string id, CancellationToken cancellationToken = new CancellationToken()) + public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (!int.TryParse(id, out int roleId)) + + if (role == null) { - return null; + throw new ArgumentNullException(nameof(role)); + } + + if (!int.TryParse(role.Id, out int roleId)) + { + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); } IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + //TODO: confirm logic + memberGroup.Name = roleName; + _memberGroupService.Save(memberGroup); + role.Name = roleName; + } + else + { + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + return Task.CompletedTask; + } + + /// + public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + //TODO: are we utilising NormalizedRoleName? + return Task.FromResult(role.NormalizedName); + } + + /// + public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + //TODO: are we utilising NormalizedRoleName and do we need to set it in the memberGroupService? + role.NormalizedName = normalizedName; + + return Task.CompletedTask; + } + + /// + public Task FindByIdAsync(string roleId, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (string.IsNullOrWhiteSpace(roleId)) + { + throw new ArgumentNullException(nameof(roleId)); + } + + if (!int.TryParse(roleId, out int id)) + { + throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Int"); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(id); return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } /// - public Task FindByNameAsync(string name, CancellationToken cancellationToken = new CancellationToken()) + public Task FindByNameAsync(string name, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (name == null) + + if (string.IsNullOrWhiteSpace(name)) { throw new ArgumentNullException(nameof(name)); } IMemberGroup memberGroup = _memberGroupService.GetByName(name); + //TODO: throw exception when not found? return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } @@ -227,10 +307,12 @@ namespace Umbraco.Cms.Core.Security return anythingChanged; } - public void Dispose() - { - //TODO: is any dispose action necessary here or is this all managed by the IOC container? - } + //TODO: is any dispose action necessary here? + + /// + /// Dispose the store + /// + public void Dispose() => _disposed = true; /// /// Throws if this class has been disposed. diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 17a45764ad..1d871e4134 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -19,7 +19,7 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MemberUserStore : UserStoreBase, string, IdentityUserClaim, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> + public class MemberUserStore : UserStoreBase, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> { private readonly IMemberService _memberService; private readonly UmbracoMapper _mapper; @@ -40,6 +40,7 @@ namespace Umbraco.Cms.Core.Security _scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider)); } + //TODO: why is this not supported? /// /// Not supported in Umbraco /// @@ -56,117 +57,140 @@ namespace Umbraco.Cms.Core.Security /// public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + // create member + IMember memberEntity = _memberService.CreateMember( + user.UserName, + user.Email, + user.Name.IsNullOrWhiteSpace() ? user.UserName : user.Name, + user.MemberTypeAlias.IsNullOrWhiteSpace() ? Constants.Security.DefaultMemberTypeAlias : user.MemberTypeAlias); + + UpdateMemberProperties(memberEntity, user); + + // create the member + _memberService.Save(memberEntity); + + if (!memberEntity.HasIdentity) + { + throw new DataException("Could not create the member, check logs for details"); + } + + // re-assign id + user.Id = UserIdToString(memberEntity.Id); + + // [from backofficeuser] we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. + // var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MembersIdentityUser.Logins)); + // TODO: confirm re externallogins implementation + //if (isLoginsPropertyDirty) + //{ + // _externalLoginService.Save( + // user.Id, + // user.Logins.Select(x => new ExternalLogin( + // x.LoginProvider, + // x.ProviderKey, + // x.UserData))); + //} + + + return Task.FromResult(IdentityResult.Success); } - - // create member - IMember memberEntity = _memberService.CreateMember( - user.UserName, - user.Email, - user.Name.IsNullOrWhiteSpace() ? user.UserName : user.Name, - user.MemberTypeAlias.IsNullOrWhiteSpace() ? Constants.Security.DefaultMemberTypeAlias : user.MemberTypeAlias); - - UpdateMemberProperties(memberEntity, user); - - // create the member - _memberService.Save(memberEntity); - - if (!memberEntity.HasIdentity) + catch (Exception ex) { - throw new DataException("Could not create the member, check logs for details"); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); } - - // re-assign id - user.Id = UserIdToString(memberEntity.Id); - - // [from backofficeuser] we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. - // var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MembersIdentityUser.Logins)); - // TODO: confirm re externallogins implementation - //if (isLoginsPropertyDirty) - //{ - // _externalLoginService.Save( - // user.Id, - // user.Logins.Select(x => new ExternalLogin( - // x.LoginProvider, - // x.ProviderKey, - // x.UserData))); - //} - - return Task.FromResult(IdentityResult.Success); } /// public override Task UpdateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); - } - - Attempt asInt = user.Id.TryConvertTo(); - if (asInt == false) - { - throw new InvalidOperationException("The user id must be an integer to work with the Umbraco"); - } - - using (IScope scope = _scopeProvider.CreateScope()) - { - IMember found = _memberService.GetById(asInt.Result); - if (found != null) + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) { - // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. - var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); - - if (UpdateMemberProperties(found, user)) - { - _memberService.Save(found); - } - - // TODO: when to implement external login service? - - //if (isLoginsPropertyDirty) - //{ - // _externalLoginService.Save( - // found.Id, - // user.Logins.Select(x => new ExternalLogin( - // x.LoginProvider, - // x.ProviderKey, - // x.UserData))); - //} + throw new ArgumentNullException(nameof(user)); } - scope.Complete(); - } + Attempt asInt = user.Id.TryConvertTo(); + if (asInt == false) + { + //TODO: should this be thrown, or an identity result? + throw new InvalidOperationException("The user id must be an integer to work with Umbraco"); + } - return Task.FromResult(IdentityResult.Success); + using (IScope scope = _scopeProvider.CreateScope()) + { + IMember found = _memberService.GetById(asInt.Result); + if (found != null) + { + // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. + var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); + + if (UpdateMemberProperties(found, user)) + { + _memberService.Save(found); + } + + // TODO: when to implement external login service? + + //if (isLoginsPropertyDirty) + //{ + // _externalLoginService.Save( + // found.Id, + // user.Logins.Select(x => new ExternalLogin( + // x.LoginProvider, + // x.ProviderKey, + // x.UserData))); + //} + } + + scope.Complete(); + + return Task.FromResult(IdentityResult.Success); + } + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// public override Task DeleteAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } - IMember found = _memberService.GetById(UserIdToInt(user.Id)); - if (found != null) + IMember found = _memberService.GetById(UserIdToInt(user.Id)); + if (found != null) + { + _memberService.Delete(found); + } + + // TODO: when to implement external login service? + //_externalLoginService.DeleteUserLogins(UserIdToInt(user.Id)); + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) { - _memberService.Delete(found); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); } - - // TODO: when to implement external login service? - //_externalLoginService.DeleteUserLogins(UserIdToInt(user.Id)); - - return Task.FromResult(IdentityResult.Success); } /// @@ -178,6 +202,16 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (string.IsNullOrWhiteSpace(userId)) + { + throw new ArgumentNullException(nameof(userId)); + } + + if (!Guid.TryParse(userId, out Guid id)) + { + throw new ArgumentOutOfRangeException(nameof(userId), $"{nameof(userId)} is not a valid x"); + } + IMember user = _memberService.GetById(UserIdToInt(userId)); if (user == null) { @@ -208,7 +242,8 @@ namespace Umbraco.Cms.Core.Security { await base.SetPasswordHashAsync(user, passwordHash, cancellationToken); - user.PasswordConfig = null; // Clear this so that it's reset at the repository level + // Clear this so that it's reset at the repository level + user.PasswordConfig = null; user.LastPasswordChangeDateUtc = DateTime.UtcNow; } @@ -216,7 +251,7 @@ namespace Umbraco.Cms.Core.Security public override async Task HasPasswordAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { // This checks if it's null - var result = await base.HasPasswordAsync(user, cancellationToken); + bool result = await base.HasPasswordAsync(user, cancellationToken); if (result) { // we also want to check empty @@ -262,8 +297,22 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(login)); } + if (string.IsNullOrWhiteSpace(login.LoginProvider)) + { + throw new ArgumentNullException(nameof(login.LoginProvider)); + } + + if (string.IsNullOrWhiteSpace(login.ProviderKey)) + { + throw new ArgumentNullException(nameof(login.ProviderKey)); + } + ICollection logins = user.Logins; - var instance = new IdentityUserLogin(login.LoginProvider, login.ProviderKey, user.Id.ToString()); + var instance = new IdentityUserLogin( + login.LoginProvider, + login.ProviderKey, + user.Id.ToString()); + IdentityUserLogin userLogin = instance; logins.Add(userLogin); @@ -280,6 +329,16 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(user)); } + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); + } + IIdentityUserLogin userLogin = user.Logins.SingleOrDefault(l => l.LoginProvider == loginProvider && l.ProviderKey == providerKey); if (userLogin != null) { @@ -311,21 +370,34 @@ namespace Umbraco.Cms.Core.Security MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); if (user == null) { - return null; + //TODO: error throw or null result? + return await Task.FromResult((IdentityUserLogin)null); + } + + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); } IList logins = await GetLoginsAsync(user, cancellationToken); UserLoginInfo found = logins.FirstOrDefault(x => x.ProviderKey == providerKey && x.LoginProvider == loginProvider); if (found == null) { - return null; + //TODO: error throw or null result? + return await Task.FromResult((IdentityUserLogin)null); } return new IdentityUserLogin { LoginProvider = found.LoginProvider, ProviderKey = found.ProviderKey, - ProviderDisplayName = found.ProviderDisplayName, // TODO: We don't store this value so it will be null + // TODO: We don't store this value so it will be null + ProviderDisplayName = found.ProviderDisplayName, UserId = user.Id }; } @@ -336,9 +408,19 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); + } + var logins = new List(); - // TODO: external login needed? + // TODO: external login needed //_externalLoginService.Find(loginProvider, providerKey).ToList(); if (logins.Count == 0) { @@ -350,7 +432,8 @@ namespace Umbraco.Cms.Core.Security { LoginProvider = found.LoginProvider, ProviderKey = found.ProviderKey, - ProviderDisplayName = null, // TODO: We don't store this value so it will be null + // TODO: We don't store this value so it will be null + ProviderDisplayName = null, UserId = found.UserId }); } @@ -358,7 +441,11 @@ namespace Umbraco.Cms.Core.Security /// public override Task AddToRoleAsync(MemberIdentityUser user, string role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); + if (cancellationToken != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + ThrowIfDisposed(); if (user == null) { @@ -443,7 +530,7 @@ namespace Umbraco.Cms.Core.Security /// /// Returns true if a user is in the role /// - public override Task IsInRoleAsync(MemberIdentityUser user, string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task IsInRoleAsync(MemberIdentityUser user, string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -452,22 +539,28 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(user)); } - return Task.FromResult(user.Roles.Select(x => x.RoleId).InvariantContains(normalizedRoleName)); + if (string.IsNullOrWhiteSpace(roleName)) + { + throw new ArgumentNullException(nameof(roleName)); + } + + return Task.FromResult(user.Roles.Select(x => x.RoleId).InvariantContains(roleName)); } /// /// Lists all users of a given role. /// - public override Task> GetUsersInRoleAsync(string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task> GetUsersInRoleAsync(string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (normalizedRoleName == null) + + if (string.IsNullOrWhiteSpace(roleName)) { - throw new ArgumentNullException(nameof(normalizedRoleName)); + throw new ArgumentNullException(nameof(roleName)); } - IEnumerable members = _memberService.GetMembersByMemberType(normalizedRoleName); + IEnumerable members = _memberService.GetMembersByMemberType(roleName); IList membersIdentityUsers = members.Select(x => _mapper.Map(x)).ToList(); @@ -475,18 +568,23 @@ namespace Umbraco.Cms.Core.Security } /// - protected override Task> FindRoleAsync(string normalizedRoleName, CancellationToken cancellationToken) + protected override Task FindRoleAsync(string roleName, CancellationToken cancellationToken) { - IMemberGroup group = _memberService.GetAllRoles().SingleOrDefault(x => x.Name == normalizedRoleName); - if (group == null) + if (string.IsNullOrWhiteSpace(roleName)) { - return Task.FromResult((IdentityRole)null); + throw new ArgumentNullException(nameof(roleName)); } - return Task.FromResult(new IdentityRole(group.Name) + IMemberGroup group = _memberService.GetAllRoles().SingleOrDefault(x => x.Name == roleName); + if (group == null) + { + return Task.FromResult((IdentityRole)null); + } + + return Task.FromResult(new IdentityRole(group.Name) { //TODO: what should the alias be? - Id = @group.Id.ToString() + Id = group.Id.ToString() }); } @@ -523,7 +621,7 @@ namespace Umbraco.Cms.Core.Security { if (user != null) { - //TODO: when to + //TODO: implement //user.SetLoginsCallback(new Lazy>(() => _externalLoginService.GetAll(UserIdToInt(user.Id)))); } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index ed77c63afc..3b78bc1b39 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -162,7 +162,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Name = "testname" }; var fakeCancellationToken = new CancellationToken() { }; - + // act IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); @@ -247,7 +247,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x=>x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); _mockMemberGroupService.Verify(x => x.GetById(777)); _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -257,23 +257,23 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") + var fakeRole = new IdentityRole("fakeGroupName") { Id = "777" }; int fakeRoleId = 777; - IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && - m.CreatorId == 123 && - m.Id == 777); + IMemberGroup fakeMemberGroup = new MemberGroup() + { + Name = "fakeGroupName", + CreatorId = 123, + Id = 777 + }; - var fakeCancellationToken = new CancellationToken() { }; - - _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(mockMemberGroup); + _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); // act - IdentityRole actual = await sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); + IdentityRole actual = await sut.FindByIdAsync(fakeRole.Id); // assert Assert.AreEqual(fakeRole.Name, actual.Name); @@ -293,7 +293,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Name = "testname" }; var fakeCancellationToken = new CancellationToken() { }; - + // act Task actual = sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); @@ -342,7 +342,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // act Action actual = () => sut.FindByNameAsync(fakeRole.Name, fakeCancellationToken); - + // assert Assert.That(actual, Throws.ArgumentNullException); _mockMemberGroupService.VerifyNoOtherCalls(); @@ -379,10 +379,6 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // act Task actual = sut.GetRoleIdAsync(fakeRole, fakeCancellationToken); - // assert - Assert.That(actual, Throws.ArgumentNullException); - - // assert Assert.AreEqual(fakeRoleId, actual.Result); }