User manager related updates (umbraco#9935)

* Only allow not-admins to assign groups they have themselves

* Only admins is allowed to change password of other admins

* Fixed issue with deep clone of UserGroup. The Allowed sections was not cloned. This resulted in the allowed sections of the object stored in cache was updated, everytime we changed the allowed sections on an object cloned from the cache. Even if we did not save it.

* Only Admins are allowed to add sections to a user group, that they don't have access to themselves

* Align backend code with UI. User managers that is are not admin, can only assign the same groups new users, that they have themselves.

* Make existingGroupAliases and empty array when creating a new user

Co-authored-by: Mole <[email protected]>

src/Umbraco.Core/Models/Membership/UserGroup.cs

@@ -19,7 +19,7 @@ public class UserGroup : EntityBase, IUserGroup, IReadOnlyUserGroup
         private string _icon;
         private string _name;
         private IEnumerable<string> _permissions;
-        private readonly List<string> _sectionCollection;
+        private List<string> _sectionCollection;
 
         //Custom comparer for enumerable
         private static readonly DelegateEqualityComparer<IEnumerable<string>> StringEnumerableComparer =
@@ -101,7 +101,10 @@ public IEnumerable<string> Permissions
             set => SetPropertyValueAndDetectChanges(value, ref _permissions, nameof(Permissions), StringEnumerableComparer);
         }
 
-        public IEnumerable<string> AllowedSections => _sectionCollection;
+        public IEnumerable<string> AllowedSections
+        {
+            get => _sectionCollection;
+        }
 
         public void RemoveAllowedSection(string sectionAlias)
         {
@@ -121,5 +124,16 @@ public void ClearAllowedSections()
         }
 
         public int UserCount { get; }
+
+        protected override void PerformDeepClone(object clone)
+        {
+
+            base.PerformDeepClone(clone);
+
+            var clonedEntity = (UserGroup)clone;
+
+            //manually clone the start node props
+            clonedEntity._sectionCollection = new List<string>(_sectionCollection);
+        }
     }
 }

src/Umbraco.Tests/Models/UserGroupTests.cs

@@ -0,0 +1,80 @@
+using System;
+using System.Diagnostics;
+using System.Linq;
+using NUnit.Framework;
+using Umbraco.Core.Composing;
+using Umbraco.Core.Models.Membership;
+using Umbraco.Core.Serialization;
+using Umbraco.Tests.TestHelpers;
+
+namespace Umbraco.Tests.Models
+{
+    [TestFixture]
+    public class UserGroupTests
+    {
+        [SetUp]
+        public void Setup()
+        {
+            Current.Reset();
+            Current.UnlockConfigs();
+            Current.Configs.Add(SettingsForTests.GetDefaultGlobalSettings);
+            Current.Configs.Add(SettingsForTests.GetDefaultUmbracoSettings);
+        }
+
+        [Test]
+
+        public void Can_Deep_Clone()
+        {
+            var item = Build();
+
+            var clone = (UserGroup)item.DeepClone();
+
+            Assert.AreNotSame(clone, item);
+            Assert.AreEqual(clone, item);
+
+            Assert.AreEqual(clone.AllowedSections.Count(), item.AllowedSections.Count());
+            Assert.AreNotSame(clone.AllowedSections, item.AllowedSections);
+
+            //Verify normal properties with reflection
+            var allProps = clone.GetType().GetProperties();
+            foreach (var propertyInfo in allProps)
+            {
+                Assert.AreEqual(propertyInfo.GetValue(clone, null), propertyInfo.GetValue(item, null));
+            }
+        }
+
+        [Test]
+        public void Can_Serialize_Without_Error()
+        {
+            var ss = new SerializationService(new JsonNetSerializer());
+
+            var item = Build();
+
+            var result = ss.ToStream(item);
+            var json = result.ResultStream.ToJsonString();
+            Debug.Print(json);
+        }
+
+        private UserGroup Build()
+        {
+            var item = new UserGroup()
+            {
+                Id = 3,
+                Key = Guid.NewGuid(),
+                UpdateDate = DateTime.Now,
+                CreateDate = DateTime.Now,
+                Name = "Test",
+                Alias = "alias",
+                Icon = "icon",
+                Permissions = new []{"a", "b", "c"},
+                DeleteDate = null,
+                StartContentId = null,
+                StartMediaId = null,
+            };
+            item.AddAllowedSection("A");
+            item.AddAllowedSection("B");
+
+            return item;
+        }
+    }
+}

src/Umbraco.Tests/Umbraco.Tests.csproj

@@ -145,6 +145,7 @@
     <Compile Include="Models\PathValidationTests.cs" />
     <Compile Include="Models\PropertyTests.cs" />
     <Compile Include="Models\RangeTests.cs" />
+    <Compile Include="Models\UserGroupTests.cs" />
     <Compile Include="Models\VariationTests.cs" />
     <Compile Include="Persistence\Mappers\MapperTestBase.cs" />
     <Compile Include="Persistence\Repositories\DocumentRepositoryTest.cs" />

src/Umbraco.Tests/Web/Controllers/UserEditorAuthorizationHelperTests.cs

@@ -116,6 +116,37 @@ public void Can_Grant_Group_Membership_With_Being_A_Member()
             Assert.IsTrue(result.Success);
         }
 
+        [Test]
+        [TestCase(Constants.Security.AdminGroupAlias, Constants.Security.AdminGroupAlias, ExpectedResult = true)]
+        [TestCase(Constants.Security.AdminGroupAlias, "SomethingElse", ExpectedResult = true)]
+        [TestCase(Constants.Security.EditorGroupAlias, Constants.Security.AdminGroupAlias, ExpectedResult = false)]
+        [TestCase(Constants.Security.EditorGroupAlias, "SomethingElse", ExpectedResult = false)]
+        [TestCase(Constants.Security.EditorGroupAlias, Constants.Security.EditorGroupAlias, ExpectedResult = true)]
+        public bool Can_only_add_user_groups_you_are_part_of_yourself_unless_you_are_admin(string groupAlias, string groupToAdd)
+        {
+            var currentUser = Mock.Of<IUser>(user => user.Groups == new[]
+            {
+                new ReadOnlyUserGroup(1, "CurrentUser", "icon-user", null, null, groupAlias, new string[0], new string[0])
+            });
+            IUser savingUser = null; // This means it is a new created user
+
+            var contentService = new Mock<IContentService>();
+            var mediaService = new Mock<IMediaService>();
+            var userService = new Mock<IUserService>();
+            var entityService = new Mock<IEntityService>();
+
+            var authHelper = new UserEditorAuthorizationHelper(
+                contentService.Object,
+                mediaService.Object,
+                userService.Object,
+                entityService.Object,
+                AppCaches.Disabled);
+
+            var result = authHelper.IsAuthorized(currentUser, savingUser, new int[0], new int[0], new[] { groupToAdd });
+
+            return result.Success;
+        }
+
         [Test]
         public void Can_Add_Another_Content_Start_Node_On_User_With_Access()
         {

src/Umbraco.Web/Editors/Filters/UserGroupEditorAuthorizationHelper.cs

@@ -76,18 +76,15 @@ public Attempt<string> AuthorizeGroupAccess(IUser currentUser, params string[] g
         /// <summary>
         /// Authorize that the user is not adding a section to the group that they don't have access to
         /// </summary>
-        /// <param name="currentUser"></param>
-        /// <param name="currentAllowedSections"></param>
-        /// <param name="proposedAllowedSections"></param>
-        /// <returns></returns>
-        public Attempt<string> AuthorizeSectionChanges(IUser currentUser,
-            IEnumerable<string> currentAllowedSections,
+        public Attempt<string> AuthorizeSectionChanges(
+            IUser currentUser,
+            IEnumerable<string> existingSections,
             IEnumerable<string> proposedAllowedSections)
         {
             if (currentUser.IsAdmin())
                 return Attempt<string>.Succeed();
 
-            var sectionsAdded = currentAllowedSections.Except(proposedAllowedSections).ToArray();
+            var sectionsAdded = proposedAllowedSections.Except(existingSections).ToArray();
             var sectionAccessMissing = sectionsAdded.Except(currentUser.AllowedSections).ToArray();
             return sectionAccessMissing.Length > 0
                 ? Attempt.Fail("Current user doesn't have access to add these sections " + string.Join(", ", sectionAccessMissing))

src/Umbraco.Web/Editors/Filters/UserGroupValidateAttribute.cs

@@ -58,13 +58,9 @@ public override void OnActionExecuting(HttpActionContext actionContext)
                         return;
                     }
 
-                    //map the model to the persisted instance
-                    Mapper.Map(userGroupSave, persisted);
                     break;
                 case ContentSaveAction.SaveNew:
-                    //create the persisted model from mapping the saved model
-                    persisted = Mapper.Map<IUserGroup>(userGroupSave);
-                    ((UserGroup)persisted).ResetIdentity();
+                    persisted = new UserGroup();
                     break;
                 default:
                     actionContext.Response = actionContext.Request.CreateErrorResponse(HttpStatusCode.NotFound, new ArgumentOutOfRangeException());

src/Umbraco.Web/Editors/PasswordChanger.cs

@@ -1,5 +1,6 @@
 using System;
 using System.ComponentModel.DataAnnotations;
+using System.Linq;
 using System.Threading.Tasks;
 using System.Web;
 using System.Web.Http.ModelBinding;
@@ -84,6 +85,11 @@ public async Task<Attempt<PasswordChangedModel>> ChangePasswordWithIdentityAsync
                     return Attempt.Fail(new PasswordChangedModel { ChangeError = new ValidationResult("The current user is not authorized", new[] { "resetPassword" }) });
                 }
 
+                if (!currentUser.IsAdmin() && savingUser.IsAdmin())
+                {
+                    return Attempt.Fail(new PasswordChangedModel { ChangeError = new ValidationResult("The current user cannot change the password for the specified user", new[] { "resetPassword" }) });
+                }
+
                 //ok, we should be able to reset it
                 var resetToken = await userMgr.GeneratePasswordResetTokenAsync(savingUser.Id);
                 var newPass = passwordModel.NewPassword.IsNullOrWhiteSpace()
@@ -246,7 +252,7 @@ public Attempt<PasswordChangedModel> ChangePasswordWithMembershipProvider(string
                 return Attempt.Fail(new PasswordChangedModel { ChangeError = new ValidationResult("Cannot set an empty password", new[] { "value" }) });
             }
 
-            //without being able to retrieve the original password, 
+            //without being able to retrieve the original password,
             //we cannot arbitrarily change the password without knowing the old one and no old password was supplied - need to return an error
             if (passwordModel.OldPassword.IsNullOrWhiteSpace() && membershipProvider.EnablePasswordRetrieval == false)
             {