FIX(server, client): Fix ACL write and traverse permissions #6512
Conversation
Hartmnt
force-pushed
the
fix_traverse
branch
3 times, most recently
from
057b94d to
3599093
Compare
Hartmnt
changed the title
Hartmnt
force-pushed
the
fix_traverse
branch
from
6e7afd0 to
c715a8c
Compare
|
@Krzmbrzl Do you think backporting this is viable? I think it is and potential breakage is small. |
|
@Krzmbrzl gentle ping :) |
src/ACL.cpp
Outdated
| bool applyFromParent = (ch != chan && acl->bApplySubs); | ||
|
|
||
| // Not using applyFromAny will ignore the context flags for the current ACL | ||
| bool applyFromAny = applyFromSelf || applyFromParent; |
There was a problem hiding this comment.
I suppose we can short-circuit in case !applyFromAny and just jump to the next loop iteration?
That should make explicit checking of this boolean further down redundant 🤔
|
Sorry for having taken so long to respond. This ACL stuff is always a bit tricky and messing it up can be rather bad so I didn't want to review without taking a proper look. But that takes time which I didn't find until now 🤷
Yeah, I think we can backport this. It fixes a bug after all and as such is eligible for backporting. And as to potentially introducing a new bug, that's a risk we have to live with unless we never release bug fix releases :) |
src/ACL.cpp
Outdated
| if (applyFromAny && (acl->pAllow & Traverse)) { | ||
| traverse = true; | ||
| if (acl->pDeny & Traverse) | ||
| } | ||
| if (applyDenyTraverse && (acl->pDeny & Traverse)) { | ||
| traverse = false; | ||
| if (acl->pAllow & Write) | ||
| } |
There was a problem hiding this comment.
This can be written as
traverse = applyFromAny && (acl->pAllow & Traverse);
traverse &= !applyDenyTraverse || !(acl->pDeny & Traverse);There was a problem hiding this comment.
I agree with the write boolean. But writing traverse as two lines of rather hard to follow logical operations seems counterproductive to me.
Previously, both the traverse and write ACL would be evaluated without taking the "in this channel" and "in sub-channels" context options into account. This would lead to denying traverse for channels that were actually supposed to be traversable. This commit refactors the ACL calculation for readability and makes sure the write and traverse checks are actually taking the context into account. Fixes mumble-voip#3579
Since 2a9dcfd and 62b1536 the Mumble server would overwrite the current channel Write ACL, if the user had Write ACL permission in the parent channel. Supposedly, this was done because otherwise malicious users could create temporary "ungovernable" channels by locking admins out denying Write ACL for them. However, this makes ACL management a lot less intuitive with regard to the Write permission. This commit reverts those commits and instead adds a check to see if the user has Write permission in the root channel instead. The reasoning being: If the server owner grants Write ACL on root, they probably want those users to be able to moderate every channel. If instead the server owner only grants Write on part of the channel tree, normal ACL rules apply and users may lock other users out for whatever reason.
Previously, it was possible to have both context checkboxes disabled in the ACLEditor, leaving the ACL entry in a dangleing inactive state. This commit makes sure, that at least one of the checkboxes is always enabled.
|
@Krzmbrzl Gentle ping for a final review. Just implemented your suggestions (except the last one). |
FIX(server): Make sure context is applied to traverse and write ACL
Previously, both the traverse and write ACL would be evaluatedwithout taking the "in this channel" and "in sub-channels" context options into account.
This would lead to denying traverse for channels that were actually supposed to be traversable.
This commit refactors the ACL calculation for readability and makes sure the write and traverse checks are actually taking the context into account.
Fixes #3579
FIX(server, client): Remove "Write" ACL parent channel inheritance
Since 2a9dcfd and 62b1536 the Mumble server would overwrite the current channel Write ACL, if the user had Write ACL permission in the parent channel.
Supposedly, this was done because otherwise malicious users could create temporary "ungovernable" channels by locking admins out denying Write ACL for them.
However, this makes ACL management a lot less intuitive with regard to the Write permission.
This commit reverts those commits and instead adds a check to see if the user has Write permission in the root channel instead.
The reasoning being: If the server owner grants Write ACL on root, they probably want those users to be able to moderate every channel.
If instead the server owner only grants Write on part of the channel tree, normal ACL rules apply and users may lock other users out for whatever reason.
FIX(client): Prevent unchecking both ACL context checkboxes
Previously, it was possible to have both context checkboxes disabled in the ACLEditor, leaving the ACL entry in a dangleing inactive state.
This commit makes sure, that at least one of the checkboxes is always enabled.