U4 8632 - Back-office user groups (v2)

[AndyButland]

Here's a second PR for user groups with some changes as discussed from the previous - primarily the removal of the one-to-one relationship of users to user types (and replacement with one-to-many relationship of users to user types) and the removal of the ability to set permissions on a user basis.

Consists of:

• Slash and burn operation to remove user types and user level permissions. There's certainly some backward compatibility issues to consider here (to start with, the User model required a type in it's constructor that no longer exists, and some obsoleted APIs have had signatures changed).
• Migration (targeted for 7.6) that:
  • Creates new tables for user groups, their relation to users and permissions
  • Migrates the data for user types into groups
  • Creates a group for each user that has specific permissions set, migrates permissions to it and adds user to it
  • Deletes the old tables (clearly needs some further careful testing!)
  • The migration has only been written for and tested on SQL Express (and then only a step at a time). It'll need checking in a true migration and possibly some syntax updates for CE and MySQL
• Management screens updated/created for users, user groups and user group permissions
• Permission checks updated to take account of groups.
  • The logic for this is to take the most permissive set of permissions the user has based on their groups. In short, if for a given node they have directly assigned or default permissions for "A" only from one group, and "B" only from another, they'll have permissions for "A" and "B"
  • Similarly for apps/sections - if a user is in a group that has access to "content", and another for "media", they can access "content" and "media"

[zpqrtbnk]

Looks like it is breaking one test?

[AndyButland]

Not when I run it locally oddly. But have pushed a change that would get around the reported error, so will see if that resolves it on CI.

[zpqrtbnk]

Nope :-) Same player... try again!

[AndyButland]

Third time lucky I think...

[zpqrtbnk]

Yup!

[nul800sebastiaan]

Really good stuff here!

I have some changes to the upgrade as there's some things not quite working:

First off, with this code I can't log in any more to start the upgrader, I think this should be okay in src/Umbraco.Core/Persistence/Repositories/UserRepository.cs / GetGroupsForUser:

var tables = SqlSyntax.GetTablesInSchema(ApplicationContext.Current.DatabaseContext.Database).ToArray();
if (tables.InvariantContains("umbracoUserGroup") == false)
	return new List<IUserGroup>();

Ideally this check would only run when we're upgrading from a version lower than 7.6.0 but I haven't looked into how to do that yet.

Then there's one upgrade query that doesn't work in SQL CE, which can be fixed like so in src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenSixZero/AddUserGroupTables.cs b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenSixZero/AddUserGroupTables.cs / MigrateUserTypesToGroups (// Add each user to the group created from their type):

SELECT u.Id, ug.id FROM umbracoUserGroup ug 
	INNER JOIN umbracoUserType ut ON ut.userTypeAlias = ug.userGroupAlias 
	INNER JOIN umbracoUser u ON ut.id = u.userType
	WHERE u.userType = ut.id");

Finally, there's a foreign key contraint on the umbracoUser.userType column so that needs to be dropped before deleting the colum in DeleteOldTables:

var constraints = SqlSyntax.GetConstraintsPerColumn(Context.Database).Distinct().ToArray();
if (constraints.Any(x => x.Item1.InvariantEquals("umbracoUser") && x.Item3.InvariantEquals("FK_umbracoUser_umbracoUserType_id")))
	Delete.ForeignKey("FK_umbracoUser_umbracoUserType_id").OnTable("umbracoUser");

Delete.Column("userType").FromTable("umbracoUser");

I did an admittedly rough merge between dev-7.6 and your code. The biggest problem is EditUser.aspx.cs where a lot of validations have been added recently. I don't know if it just my merged branch now but it's not possible to create a user. Could you check in your branch if that's a problem too?

Then I got to play with this a bit and with regards to user groups/permissions and combining them that worked alright. I do notice something and I'm not sure if that's on purpose: if I add someone to the group Editors and Writers then they have (for example) publish permissions, even though a Writer by default has no such permissions. Are you just combining all permissions?

Another one that was a bit puzzling was that setting tree permissions doesn't really set them correctly, for example a user is in the group writers and editors. If I set a node to have only Browse Node permissions for the group Writers then I still get quite a few permissions inherited from the Editors group:

However, when I then update the user group permissions for the Editor group, the permissions get combined again. I don't know if there's a good solution. Maybe the solution is: when you set node specific permissions then the minimal selected permissions for all groups combined should always apply. So if I set the Write group to only allow Browse then even if I'm also in the Editors group, I can only browse.

Finally, it is painfully clear that we need a "Reset permissions to default for this user group". I guess we always needed that for the current implementation as well. Currently there's no way (without going to database) to remove node specific permissions.

[nul800sebastiaan]

Re-read your comments:

• Queries should now work for both SQL CE (checked) and MySQL (have yet to check, maybe @zpqrtbnk can do so as he has test databases)
• "most permissive set of permissions" - alright, let's stick with that for now, seems only fair :)

[nul800sebastiaan]

Aha, the node-specific permissions can already be reset by going to the node, right-clicking it and using the "Permissions" dialog. I didn't even know that existed.. haha! Forget about that one!

[AndyButland]

Me neither... thought I'd finished and then discovered that screen!

You're right, there were some issues with creating new users and groups. Should be fixed with the latest commit to the PR. Thanks for looking over this.

[WimBoom]

In wich Umbraco version will this pull request implemented? I hope Umbraco 7.6?
This functionality is required for a client, with a lot of authors... So setting
rights at group level is required.

[Shazwazza]

This will be included in 7.7. 7.6 is more or less locked. 7.7 will focus on user group security and an update to it's UX. The problem currently is that the UX for managing these permissions is very old and needs to be updated so we want to get that all done at once.

[JimBobSquarePants]

Can sections be null? The method GetSections() returns an IEnumerable and there's no null check against it in other calls.

[AndyButland]

I don't recall finding that, but the change I've made here isn't actually adding the null check, it's adding the "is empty" check. If I remember right from a unit test I found that a non-null but empty list of sections, errored when you tried to call Max() on it. Hence the check now for "is not null and is not empty".

[JimBobSquarePants]

Sorry should have been more clear. If GetSections() always returns an enumerable then Any() would be enough. No need for the prior null check. I'd boy scout it if so to make the code easier for the next visitor.

[AndyButland]

Got you, and you're right, it's not necessary. Removed.

[Shazwazza]

Hi @AndyButland I'm going to close this one now as I've merged this into a 7.6 branch and pushed a new one to our repo with the name 'temp-U4-8632', you can follow along on this branch if you wish and this is where i'll be making any modifications, etc...

Thanks!!

[Shazwazza]

Wat!? @AndyButland you created a new obsolete legacy class? why? I'm just removing this now since we don't need it. maybe you were just being extra thorough?

[AndyButland]

Been a while now... but think I was probably just following existing patterns. There was likely the analogous class for User or UserType, and if in use - even if obsolete - probably thought needed to be there (but with the same warning that it'll likely be going soon and there are better public methods to use). If you are overhauling this now anyway sure there's no reason now to keep it.