diff --git a/src/ACL.cpp b/src/ACL.cpp index 8d3180ce51..f745dcaa96 100644 --- a/src/ACL.cpp +++ b/src/ACL.cpp @@ -133,38 +133,76 @@ QFlags< ChanACL::Perm > ChanACL::effectivePermissions(ServerUser *p, Channel *ch bool traverse = true; bool write = false; - ChanACL *acl; + // Iterate over all parent channels from root to the channel the user is in (inclusive) while (!chanstack.isEmpty()) { ch = chanstack.pop(); - if (!ch->bInheritACL) + if (!ch->bInheritACL) { granted = def; + } - foreach (acl, ch->qlACL) { + for (const ChanACL *acl : ch->qlACL) { bool matchUser = (acl->iUserId != -1) && (acl->iUserId == p->iId); bool matchGroup = Group::appliesToUser(*chan, *ch, acl->qsGroup, *p); + + bool applyFromSelf = (ch == chan && acl->bApplyHere); + bool applyInherited = (ch != chan && acl->bApplySubs); + + // Flag indicating whether the current ACL affects the target channel "chan" + bool apply = applyFromSelf || applyInherited; + + // "apply" will be true for ACLs set in the reference channel directly (applyHere), + // or from a parent channel which hands the ACLs down (applySubs). + // However, we have one ACL that needs to be evaluated differently - the Traverse ACL. + // Consider this channel layout: + // Root + // - A (Traverse denied for THIS channel, but not sub channels) + // - B + // - C + // If the user tries to enter C, we need to deny Traverse, because the user + // should already be blocked from traversing A. But "apply" will be false, + // as the "normal" ACL inheritence rules do not apply here. + // Therefore, we need applyDenyTraverse which will be true, if any channel + // from root to the reference channel denies Traverse without necessarily + // handing it down. + bool applyDenyTraverse = applyInherited || acl->bApplyHere; + if (matchUser || matchGroup) { - if (acl->pAllow & Traverse) + // The "traverse" and "write" booleans do not grant or deny anything here. + // We merely check, if we are missing traverse AND write in this + // channel and therefore abort without any permissions later on. + if (apply && (acl->pAllow & Traverse)) { traverse = true; - if (acl->pDeny & Traverse) + } + if (applyDenyTraverse && (acl->pDeny & Traverse)) { traverse = false; - if (acl->pAllow & Write) - write = true; - if (acl->pDeny & Write) - write = false; - if (ch->iId == 0 && chan == ch && acl->bApplyHere) { - if (acl->pAllow & Kick) + } + + write = apply && (acl->pAllow & Write) && !(acl->pDeny & Write); + + // These permissions are only grantable from the root channel + // as they affect the users globally. For example: You can not + // kick a client from a channel without kicking them from the server. + if (ch->iId == 0 && applyFromSelf) { + if (acl->pAllow & Kick) { granted |= Kick; - if (acl->pAllow & Ban) + } + if (acl->pAllow & Ban) { granted |= Ban; - if (acl->pAllow & ResetUserContent) + } + if (acl->pAllow & ResetUserContent) { granted |= ResetUserContent; - if (acl->pAllow & Register) + } + if (acl->pAllow & Register) { granted |= Register; - if (acl->pAllow & SelfRegister) + } + if (acl->pAllow & SelfRegister) { granted |= SelfRegister; + } } - if ((ch == chan && acl->bApplyHere) || (ch != chan && acl->bApplySubs)) { + + // Every other regular ACL is handled here + if (apply) { granted |= (acl->pAllow & ~(Kick | Ban | ResetUserContent | Register | SelfRegister | Cached)); granted &= ~acl->pDeny; } diff --git a/src/mumble/ACLEditor.cpp b/src/mumble/ACLEditor.cpp index 1bacc42dbb..9e4c4ad84a 100644 --- a/src/mumble/ACLEditor.cpp +++ b/src/mumble/ACLEditor.cpp @@ -815,18 +815,26 @@ void ACLEditor::on_qcbACLInherit_clicked(bool) { void ACLEditor::on_qcbACLApplyHere_clicked(bool checked) { ChanACL *as = currentACL(); - if (!as || as->bInherited) + if (!as || as->bInherited) { return; + } as->bApplyHere = checked; + if (!checked && !qcbACLApplySubs->isChecked()) { + qcbACLApplySubs->setCheckState(Qt::Checked); + } } void ACLEditor::on_qcbACLApplySubs_clicked(bool checked) { ChanACL *as = currentACL(); - if (!as || as->bInherited) + if (!as || as->bInherited) { return; + } as->bApplySubs = checked; + if (!checked && !qcbACLApplyHere->isChecked()) { + qcbACLApplyHere->setCheckState(Qt::Checked); + } } void ACLEditor::qcbACLGroup_focusLost() { diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp index 4f511bb10a..92c50dbf3d 100644 --- a/src/mumble/MainWindow.cpp +++ b/src/mumble/MainWindow.cpp @@ -2561,20 +2561,6 @@ void MainWindow::updateMenuPermissions() { target.channel->uiPermissions = p; } - Channel *cparent = target.channel ? target.channel->cParent : nullptr; - ChanACL::Permissions pparent = - cparent ? static_cast< ChanACL::Permissions >(cparent->uiPermissions) : ChanACL::None; - - if (cparent && !pparent) { - Global::get().sh->requestChannelPermissions(cparent->iId); - if (cparent->iId == 0) - pparent = Global::get().pPermissions; - else - pparent = ChanACL::All; - - cparent->uiPermissions = pparent; - } - ClientUser *user = Global::get().uiSession ? ClientUser::get(Global::get().uiSession) : nullptr; Channel *homec = user ? user->cChannel : nullptr; ChanACL::Permissions homep = homec ? static_cast< ChanACL::Permissions >(homec->uiPermissions) : ChanACL::None; @@ -2610,7 +2596,7 @@ void MainWindow::updateMenuPermissions() { qaChannelAdd->setEnabled(p & (ChanACL::Write | ChanACL::MakeChannel | ChanACL::MakeTempChannel)); qaChannelRemove->setEnabled(p & ChanACL::Write); - qaChannelACL->setEnabled((p & ChanACL::Write) || (pparent & ChanACL::Write)); + qaChannelACL->setEnabled((p & ChanACL::Write) || (Global::get().pPermissions & ChanACL::Write)); qaChannelLink->setEnabled((p & (ChanACL::Write | ChanACL::LinkChannel)) && (homep & (ChanACL::Write | ChanACL::LinkChannel))); diff --git a/src/murmur/Messages.cpp b/src/murmur/Messages.cpp index c92e366b07..8b1e6a5833 100644 --- a/src/murmur/Messages.cpp +++ b/src/murmur/Messages.cpp @@ -1745,8 +1745,15 @@ void Server::msgACL(ServerUser *uSource, MumbleProto::ACL &msg) { if (!c) return; - if (!hasPermission(uSource, c, ChanACL::Write) - && !(c->cParent && hasPermission(uSource, c->cParent, ChanACL::Write))) { + // For changing channel properties (the 'Write') ACL we allow two things: + // 1) As per regular ACL propagating mechanism, we check if the user has been + // granted Write in the channel they try to edit + // 2) We allow all users who have been granted 'Write' on the root channel + // to be able to edit _all_ channels, independent of actual propagated ACLs + // This is done to prevent users who have permission to create (temporary) + // channels being able to "lock-out" admins by denying them 'Write' in their + // channel effectively becoming ungovernable. + if (!hasPermission(uSource, c, ChanACL::Write) && !hasPermission(uSource, qhChannels.value(0), ChanACL::Write)) { PERM_DENIED(uSource, c, ChanACL::Write); return; }