Change required ACL needed to see user IPs to "Ban"

[edshot99]

Context

Privacy

Description

Adding a server-side configuration option that doesn't let all users on a Mumble server see each other's public IP addresses.
At the very least, only making it where server admins can only see the IP address.

Mumble component

Server

OS-specific?

No

Additional information

There is an old pull request here, but it is outdated: #73

I just changed a line of code and compile the server for myself, but it'd be nice to not have to compile every new Mumble version.

[Krzmbrzl]

We already have restrictions on who will be able to see IP addresses. The code adding the IP address to the message is

mumble/src/murmur/Messages.cpp

Line 2318 in 66e202f

msg.set_address(pDstServerUser->haAddress.toStdString());

which is guarded by details being true, which is defined as

mumble/src/murmur/Messages.cpp

Line 2256 in 66e202f

bool details = extend;

which is defined as

mumble/src/murmur/Messages.cpp

Line 2249 in 66e202f

bool extend = (uSource == pDstServerUser) || hasPermission(uSource, qhChannels.value(0), ChanACL::Register);

So in other words: you'll always see your own IP address. In addition, users having the Register permission on the root channel will also see IP addresses of other users.
The Register ACL grants the power to register other users on the server (and is therefore distinct from the SelfRegister ACL). Hence, the reasoning is that those who are allowed to register other users on the server serve as some kind of admin/moderator and can therefore see other client's IP address.

@davidebeatrici @Hartmnt it may be worth considering to instead check the Write ACL on the root channel. Though on the other hand, I don't see any reason why someone should be granted Register permission while not being a server admin or at least moderator. So maybe the current logic is fine?

[Hartmnt]

I don't see any reason why someone should be granted Register permission while not being a server admin or at least moderator. So maybe the current logic is fine?

Mhh. I guess some kind of "web-of-trust" system or maybe register service accounts could use register without having full admin rights on the server.

But I agree that showing IP with Register seems somewhat arbitrary. I guess a rational reason for being able to see the user's IP is when you have the Ban permission? Write on root seems too extreme IMO as that is effectively SuperUser equivalent.

[Krzmbrzl]

I guess a rational reason for being able to see the user's IP is when you have the Ban permission?

Yeah, I thought about that as well since banning is kinda the only administrative task for which the IP address is relevant at all.

Write on root seems too extreme IMO as that is effectively SuperUser equivalent.

True.

Personally, I'd be fine with keeping it as-is, but I would also be open to change the required ACL to something like Ban. I don't really have an opinion on which it should be. 🤷

[Hartmnt]

Let's make it ban then.

[davidebeatrici]

To be fair, knowing the IP address may also be useful for diagnostics purposes, such as tracing the route(s) to investigate a potential hiccup.

However, from my perspective, the Register privilege is a higher level compared to the Ban one. I can see cases where a moderator should be able to ban users but not register new ones.

In conclusion, I agree with the change performed in #6700.

[edshot99]

So in other words: you'll always see your own IP address. In addition, users having the Register permission on the root channel will also see IP addresses of other users. The Register ACL grants the power to register other users on the server (and is therefore distinct from the SelfRegister ACL). Hence, the reasoning is that those who are allowed to register other users on the server serve as some kind of admin/moderator and can therefore see other client's IP address.

Thanks for the explanation, didn't know that. Guess I should have tested with a non-registered account, but I just assumed that is how it was for all people since I didn't think any of the ACL permissions would trigger other functionality.

The change is still good regardless.

[Krzmbrzl]

should have tested with a non-registered account

it's not dependent on the client being registered. It's dependent on the client having the Register ACL, i.e. the client being allowed to register other users.