Backport "Merge PR #6512: FIX(server, client): Fix ACL write and trav…

…erse permissions" to 1.5.x (#6676)

src/ACL.cpp

@@ -139,38 +139,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;
 				}

src/mumble/ACLEditor.cpp

@@ -827,18 +827,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() {

src/mumble/MainWindow.cpp

@@ -2556,20 +2556,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;
@@ -2605,7 +2591,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)));

src/murmur/Messages.cpp

@@ -1756,8 +1756,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;
 	}