Open rubenseyer opened 4 years ago
streaps
commented
4 years ago The grumble server starts again and I can add an @all group. The ACL is stored correctly and survives restart of the server unharmed. :)
Unfortunately it doesn't seem to have any effect. I haven't tried all permissions, but for example "register self" does not work at all.
rubenseyer
commented
4 years ago Yeah, seems like this hasn't been working for quite a while and nobody noticed until now. The problem is that grumble never actually tells the connecting user they have the ability to register. There are a couple of fixme comments which refer to implementing this. https://github.com/mumble-voip/grumble/blob/df983754639dbe9b689f47584bffabae9f438137/cmd/grumble/server.go#L692 https://github.com/mumble-voip/grumble/blob/df983754639dbe9b689f47584bffabae9f438137/cmd/grumble/server.go#L902
It's a bigger fix than just four lines so I don't have time to look at this right now, but even though it technically is a new feature I can't imagine it to be too big. One could probably be inspired by the corresponding implementation in Murmur.
rubenseyer
commented
4 years ago I thought about it some more and couldn't resist (and besides it wasn't too many lines). The old HasPermission already traversed everything so it was just a matter of refactoring out the intermediate step of calculating the permissions from testing them.
Now, this doesn't implement ACL caching, but it doesn't come with worse performance than before anyway (because we traversed the entire tree before, too.) It wouldn't be too hard, I just didn't want to impose any design decisions on how it should be structured (but I would probably make an ACLCache type and move HasPermission et al. to methods on that, with a field on the server). I suggest benchmarks before I would bother with caching, because it would be a change that touches many lines of code, but then again "large" use-cases are not too keen on grumble for other reasons (namely no RPC).
I managed to register myself, but that was all I tested. If you come up with any weird combinations to try out please do.
streaps
commented
4 years ago Registration works Write ACL ✓
but: Groups and it's members are not saved (or not displayed) in the Groups tab. modified inherited groups appear multiple times, like:
@all <- default (server)
@all <- root
@custom <- root
@all <- sub-channel "one"
@all <- this sub-sub-channel "two"
root
+ one
+ twoNon-merged inheritance seems to be the usual Murmur behaviour (it makes sense since you might want to see where some rule is coming from).
Groups were saved (you can e.g. set ACLs on them in the dialog) but there was a problem with lookup. Seems there was a bug where the context chain for groups was never built, so grumble always returned zero groups.
Group members were usually not saved (unless you had a lowercase name) because that lookup should be case insensitive according to the Murmur implementation, and it looks like the client just sends a lowercase name. I changed the server lookup table to just use lowercase everywhere adding strings.ToLower calls.
streaps
commented
4 years ago Non-merged inheritance seems to be the usual Murmur behaviour (it makes sense since you might want to see where some rule is coming from).
You are right, I didn't test it properly. "Applies to sub-channel" was not set in the group on my Murmur server. After enabling it I see the exact same behaviour on Murmur too.
I haven't tested it extensively, but groups seem to work now.
ravicant
commented
3 years ago I dont think it working it automatically switch to superuser - https://i.ravicant.in/9Lb6Fknjp.gif
Summary (see later comments):
Played around with a few channels and some ACLs and went from "everything is SuperUser and broken" to "seems to be working" at least, but with these issues it seems like the whole of ACLs could use some testing.