Add permission for Reset Comment/Avatar

[alexscorer]

Currently if a user has a "move" permission, they can also reset the comment or avatar of another user as describe in issue #1016. I stumbled into this by accident when I gave a user the move permission to help get new users into the right channels with their friends.

After reviewing the content found in PR #1223 which introduced the reset avatar feature, I found the code that allowed a user to do these resets and changed the required permission from Move to RegisterUser as suggested by @bontibon in the issue thread. As they state, this permission makes more sense than the move permission.

After compiling I found no issues and the changes worked as expected. The one caveat is that if the server has these changes implemented and not the client, (seems daft but this is preferable for me so I don't have to ask my users to change anything) it makes the permissions required to do the reset be move AND registeruser. The client will expect the user to have Move permission and the server will expect the RegisterUser permission, hence then requiring both.

Fixes #1016

[Krzmbrzl]

I actually think that you can squash both commits together into a single one. Could you also prefix that commit's message with ACL: please?
(See other commits in the repo for how we use prefixes)

[Natenom]

If there are moderators on your server who should be able to remove comments then it is better to be bound to "move". Btw this only works if the permission is set in the root channel. A moderator can't do much with move, but anyone with "register users" is able to see all user information, including IP address and certificate AND is able to remove oder rename all existing user registrations via Server -> Userlist.

[alexscorer]

Sorry for the use of force pushes, I was struggling trying to do the commit squashing.
Both commits squashed into a singular one now, with the ACL: prefix as requested.

@Natenom Currently the Move permission has a lot of utility for my server, it's very convenient for a few privileged users to be able to do. The server doesn't have a large amount of users, 20 people maximum, so I allowed self-registration for everybody and so far there hasn't been issue. No one has needed to be registered by another user. I don't think my situation is unique either, browsing through some of the publicly listed servers shows a low population.
It could be argued that the "move" permission should be removed for anyone abusing it by removing comments/avatars, and that changing it to RegisterUser opens up more potential abuse. However, for anyone that already had the RegisterUser permission they are able to cause far more abuse, as you say they could remove all registrations if they wanted to!

[Krzmbrzl]

I think it's really silly that Mumble reuses the same ACLs for different purposes in the first place. Imo there should be a distinct ACL for every distinct action you want to be controlled. But I guess that's something for the rewrite ^^

I think there's the need for some discussion before merging this PR. See my comment in #1016

EDIT: Btw. force-pushing is no problem for PRs (in fact it's almost always necessary and we do it all the time) :)

[Krzmbrzl]

I agree with @streaps that ultimately this should be a separate ACL. Do you think you can implement that?

This would require adding the new ACL as a new flag, modifying the server to use the new ACL (as you did with this PR) and modifying the client's ACLEditor UI to also include this new ACL.

[alexscorer]

Yes I can attempt this, I will try to submit the new commits within the week. What would be the best way to submit this? My original PR was a small change so putting it all into one commit made sense but this will require many more changes, should it still be submitted as a singular commit?

TODO list mostly for myself of files that will need to be changed.

• ACLEditor.cpp
• ACLEditor.h
• ACLEditor.ui
• ACL.cpp
• ACL.h
• MainWindow.cpp
• Messages.cpp

While digging I also found an issue and PR that are related, #3861 and #2421 respectively. The PR is from 4 years ago but it seems to be a good place to start, and thankfully includes this guidance.

[Krzmbrzl]

Awesome!
Also good catch with the existing PR. We really should dig through all old PRs and sort them out sometime xD

I think you can pretty much cherry-pick the commits from that PR and then we can pick off from where they left: Thinking about what to do with backwards compat.

My suggestion would be to simply check the server's version via ServerHandler::uiVersion like this:

mumble/src/mumble/MainWindow.cpp

Line 2017 in 39c81de

if (c && g.sh && g.sh->uiVersion >= 0x010400) {

And in that if-block we can add the new permissions to the ACL window as this means that the server is running 1.4.0 or newer.

And I'd simply leave the old Move-permission alone. On old servers it'll function as this hybrid permission and on newer version it'll work as one should expect 🤷

And about the commits: Don't worry for now. Let's just implement this and once it's done we'll have a look at what changes could be grouped together and based on that we can squash commits :)

[streaps]

This means if I connect to an 1.3 server I get the current behaviour of 1.3?

I like that, it's easy to understand and has no weird side effects on upgrade.

[Krzmbrzl]

This means if I connect to an 1.3 server I get the current behaviour of 1.3?

That'd be the idea, yes. It should appear as if nothing had changed

I like that, it's easy to understand and has no weird side effects on upgrade.

Except for folks that are used to use the move privilege also being responsible for comment resetting being confused why that doesn't work anymore. But this should be pretty self-explanatory once they see that there are separate ACLs for that now 🤷

[alexscorer]

I implemented the backward compatibility check via
if ( (g.sh->uiVersion < 0x010400) && ((perm == 0x100000) || (perm == 0x200000)) ) continue;
This is placed in the for loop that iterates over the available permissions and adds the check boxes, skipping the iteration if the server version is too low and it encounters these permissions.
ACLEditor.h didn't need to be changed in the end, and as long as I haven't missed anything I have my fingers crossed this is ready to merge!
Fixes #1016
Fixes #3861

[Krzmbrzl]

After having fixed everything, please squash all changes into the single commit you already have. After that run ./scripts/updatetranslations.sh in order to include the new strings in Mumble's translation process.

And in regards to your commit message:

1. There's a typo: "...to reset another users comment or avatar..." should be "...to reset another user's comment or avatar..." (note the apostrophe)
2. "On these older servers, granting the Move permission in the root channel grants both of these new permissions." - technically that's not true and might cause confusion. I'd go with something like "On older servers the Move permission in the root channel was also used for allowing or denying comment and avatar changes."

[Krzmbrzl]

Oh and btw.: #3861 is not addressed by this PR as this is another issue. The user there didn't request for an ACL that allows to reset comments but rather an ACL that prevents users without it to set comments in the first place.

[Krzmbrzl]

I also just remembered something else: We currently have duplication of where the ACLs are defined. They are also in

mumble/src/murmur/Murmur.ice

Lines 143 to 172 in 50aea07

	/** Write access to channel control. Implies all other permissions (except Speak). */
	const int PermissionWrite = 0x01;
	/** Traverse channel. Without this, a client cannot reach subchannels, no matter which privileges he has there. */
	const int PermissionTraverse = 0x02;
	/** Enter channel. */
	const int PermissionEnter = 0x04;
	/** Speak in channel. */
	const int PermissionSpeak = 0x08;
	/** Whisper to channel. This is different from Speak, so you can set up different permissions. */
	const int PermissionWhisper = 0x100;
	/** Mute and deafen other users in this channel. */
	const int PermissionMuteDeafen = 0x10;
	/** Move users from channel. You need this permission in both the source and destination channel to move another user. */
	const int PermissionMove = 0x20;
	/** Make new channel as a subchannel of this channel. */
	const int PermissionMakeChannel = 0x40;
	/** Make new temporary channel as a subchannel of this channel. */
	const int PermissionMakeTempChannel = 0x400;
	/** Link this channel. You need this permission in both the source and destination channel to link channels, or in either channel to unlink them. */
	const int PermissionLinkChannel = 0x80;
	/** Send text message to channel. */
	const int PermissionTextMessage = 0x200;
	/** Kick user from server. Only valid on root channel. */
	const int PermissionKick = 0x10000;
	/** Ban user from server. Only valid on root channel. */
	const int PermissionBan = 0x20000;
	/** Register and unregister users. Only valid on root channel. */
	const int PermissionRegister = 0x40000;
	/** Register and unregister users. Only valid on root channel. */
	const int PermissionRegisterSelf = 0x80000;

And thus this also has to be updated ☝️

[alexscorer]

Thanks for spotting these many mistakes @Krzmbrzl, I should stop editing after a certain time in the night 👍
I have now corrected each instance of incorrect indentation and typos, added comments, the additional check for the new version, changes to the .ice file and added the translation.
I also tested this on both a 1.4.0 server and 1.3.1 to ensure it's giving the correct behaviour.
I look forward to any additional feedback that can be provided :)

[streaps]

I didn't realize that there are now two new ACLs. I thought the proposal was to add one ACL for resetting Text and Avatar. This is getting way to fine-grained (IMHO). What is the use-case for that?

[alexscorer]

I didn't realize that there are now two new ACLs. I thought the proposal was to add one ACL for resetting Text and Avatar. This is getting way to fine-grained (IMHO). What is the use-case for that?

I think it's better to have both, it's more explicit for the administrator, ACL is occasionally regarded as being complicated already. As for the use-case, I have a some ideas for my own server that will place more importance on avatars than comments. There's also already this kind of fine-grained control already in the ACL system, in the form of the traverse vs enter permissions.

I don't think the ACL being fine-grained is bad either, it's the kind of thing an administrator typically desires. The only problem I can imagine for the user is making the UI a bit more cramped but it's only 2 extra rows in the permission list.

[streaps]

But it doesn't stop there. Now we have the situation that in some PR it was decided ad-hoc that ACL should be as fine grained as possible. Now others have other needs and want to split up other permissions too. With how many permissions will we users have to deal with (an admin is also a user and not every admin is comfortable in juggling endless permissions).

This super-fine granularity is inconsistent with the permissions we have now.

It's also affects other Mumble clients, servers and remote management apps.

So why don't we have a separate permission for:
change my own Avatar
change my own Comment
reset Avatar for others
reset Comments for others
change Avatar for others
change Comments for others

I also would like to see a "Write ACL" permission for every single permission.

Write "Write" ACL
Write "Traverse" ACL
Write "Enter" ACL

I think that would be really useful for some use cases. ;)

You wrote you have some ideas, but you didn't describe your use case and why exactly you think one new permission is not enough.

[alexscorer]

So why don't we have a separate permission for:
change Avatar for others
change Comments for others

I also would like to see a "Write ACL" permission for every single permission.

Write "Write" ACL
Write "Traverse" ACL
Write "Enter" ACL

I think that would be really useful for some use cases. ;)

You wrote you have some ideas, but you didn't describe your use case and why exactly you think one new permission is not enough.

I originally added 2 new permissions instead of one in this PR because as mentioned by @mkrautz in #2421

Also, shouldn't Remove Avatar and Change Comment be separate permissions?

However if you feel this strongly that it should be only one permission and others agree then it can be modified to only be one permission. @Krzmbrzl did make a comment suggesting to use the ban permission, so perhaps it could even be reduced back to 0 new permissions? My primary concern is getting this functionality out of the move permission.

You mentioned being able to change others avatars/comments, however as mentioned by @bontibon in the original issue for adding a method to remove avatars comments #1218 :

I would say this is good behaviour. I would much rather my comment get cleared by a server owner than modified to say something I did not write.

As such I didn't ask it about, it would only complicate what this PR is trying to do, which as stated is to get this functionality out of the move permission. However, perhaps you can implement a PR doing this so it can be discussed? Opinions may have changed over time :)

I'm confused about what you mean by having "Write ACL" for every permission?

As for my own ideas, one of them was to replace the icon to the left of the users username with a scaled down version of their avatar, I didn't go into them because I didn't think it's important and I've done shockingly little work on it so far :)

[streaps]

I see, my ironic rambling is too confusing.

My point is: there should be a good reason to introduce a new permission or make changes. I think there is agreement, that the comment/avatar reset should not be linked to the Move permission. It could be under the Kick or Ban permission, which both are global permissions too.

It could also be a new permission (I forgot why I thought it is a good idea to have an own permission for it). I just don't see why we need two new permissions. The original idea was that it should be possible to remove problematic avatars, like it was already possible to remove problematic comments. I cannot come up with a scenario where it would be useful to give a moderator the ability to remove comments but not avatars (or the other way around).It also could be called Reset User Profile (or something similar).

[Krzmbrzl]

I think using separate permissions for separate actions is the way to go. I already dislike that some permissions are used for other stuff as this is very counter-intuitive and also a mess for maintenance as you get all sorts of side-effects when granting/denying permission X.

In regards to users getting confused, I think this is then a flaw of the UI. Right now the way ACLs are presented to the user is quite messy (also the way they are implemented but that's another issue) and definitely needs some workup at some point. However the problem is not that there are too many permissions (imo).

[Krzmbrzl]

LGTM. Only some minor stuff in the comments. You can simply overtake my suggestions and then squash all commits back into a single one :)

[streaps]

I think using separate permissions for separate actions is the way to go.

If that means that a permission for reset avatar and reset comment is needed than that would imply that we should split everything up into a hundred permissions. changing my own comment is an action, changing my own avatar is an action. Then there are passive actions: receiving text, receiving audio, receiving the user list, receiving the channel list, ...

Adding super-fine-grained permissions in this case – just because it would be better in the first place – would be some half-assed solution without a bigger plan.

I'm not against fine-grained permissions on the atomic level in general, but it would need a different and better admin UI. Now it only adds complexity without any real value. Nobody needs to configure avatar and comment reset separately. Did anybody ask for it, because it's essential for their setup?

Please don't add unnecessary cruft to the UI.

[Krzmbrzl]

but it would need a different and better admin UI

I agree (as written above).

Now it only adds complexity without any real value.

I think 2 more rows should be manageable. It's not like this PR adds 200 new permissions.

The fact that the UI has not been overhauled yet, shouldn't mean that we can't start improving stuff somewhere else already. That'd totally break the development flow.

[Krzmbrzl]

That being said though, in the end I don't really care whether it ends up being 1 permission or 2. I think either works though from a purely conceptual point of view I'd prefer 2 🤷

[alexscorer]

Suggestions have been committed and then squashed as requested, thank you for all of the help @Krzmbrzl :)

[Krzmbrzl]

Before merging this: Does someone else have similar concerns about making this 2 instead of 1 new ACL (as voiced by @streaps) ?

/cc @toby63

[toby63]

Am I mentioned because I'm such a grinch 😛 😄

I agree with @streaps in two things:

1. that this should not become the new standard procedure (to add as many permissions as possible, without UI changes)
2. that in this specific case there is really no reason why the permissions should be split.

So for a compromise:
Would it be possible to have two permissions in code, but only one permission in the UI for now?
I guess that should be possible, right?
Sry @alexscorer if that causes work.
This way we could implement more fine-grained options in the UI later.

For the new UI, i imagine a system like this:
We make a pyramid system.
So behind the scenes and in extended options we have all permission options (bottom of the pyramid).
But in standard UI (and by default) we have only a few options (like maybe 10) that combine multiple permission options (top of the pyramid).

[Krzmbrzl]

In principle I think the proposed compromise sounds good, however it'd involve a bit of special case handling in the UI code and I think it's not really worth the additional work.

Let's go with a single permission for both and call it ResetUserContent or something like that.

@alexscorer sorry that we didn't have this discussion earlier, but I think it shouldn't be a lot of work to squash the 2 new commits into a single one. Could you do that, please?

EDIT:

Am I mentioned because I'm such a grinch?

Haha no. I mentioned you because you are very active on GitHub (and thus likely to answer in time) and also not shy of expressing your opinion ;)

[alexscorer]

I'll confess, I went with the easier method that @Krzmbrzl proposed rather than try to make a new "umbrella" permission, although it sounded better. Sorry @toby63 but perhaps it's something I or someone else can try to implement later :)
All previous ResetComment and ResetTexture have been replaced with ResetUserContent, with appropriate changes being made to any strings, commit messages etc too. I once again did a test compile, connected to a 1.3 server to make sure the conditional check still works. All seems to function as expected :)
Please comment if anyone has any additional feedback or I've missed something.

[Krzmbrzl]

Thank you for the rework! :)

@streaps, @toby63 I guess the discussion is resolved by this then?

[Krzmbrzl]

Thank you very much for your contribution! :)