Private message

[lunyav]

This will resolve #7010.

Checks

• My commits follow the commit guidelines
  Forgot to commit the translations on their own tho.

Are there any docs Id have to adjust?

[coderabbitai]

Walkthrough

A new PrivateMessage permission (0x1000) is added to the ChanACL::Perm enum. The permission is integrated into the ACL editor with version gating for servers 1.7.0 and later. The TextMessage permission check for direct channel recipients is replaced with a check for either PrivateMessage or Write permissions. The ACL UI is updated to display the new permission, and the MainWindow logic is modified to use PrivateMessage instead of TextMessage for determining whether users can send private messages in servers supporting this permission.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Private message' is concise and directly reflects the main change: adding a new PrivateMessage permission to allow granular control over private messaging.
Description check	✅ Passed	The description includes a reference to the linked issue (#7010), confirms compliance with commit guidelines, and asks clarifying questions about documentation needs.
Linked Issues check	✅ Passed	The changes fully implement the requirement from #7010 by adding a new PrivateMessage permission distinct from TextMessage, enabling granular control over private messaging across both client and server components.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the objective of implementing a granular PrivateMessage permission; no out-of-scope modifications were detected in the affected files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

src/ACL.cpp (1)

374-375: Use a user-facing label style consistent with other permissions.

permName() currently returns PrivateMessage, while other labels are human-readable (with spaces). This will look inconsistent in the ACL UI.

Suggested tweak

-		case PrivateMessage:
-			return tr("PrivateMessage");
+		case PrivateMessage:
+			return tr("Private message");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ACL.cpp` around lines 374 - 375, permName() returns "PrivateMessage" for
the PrivateMessage enum case which is inconsistent with other human-readable
permission labels; update the PrivateMessage branch in ACL.cpp to return a
spaced, user-facing label (e.g., return tr("Private Message")) so it matches the
style used by the other permission cases and the ACL UI.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/mumble/ACLEditor.cpp`:
- Line 112: The loop upper bound in ACLEditor.cpp (the for loop using ((iId ==
0) ? 31 : 17)) lets i reach 16 for non-root channels, which exposes the
root-only ChanACL::Kick bit; change the non-root branch to 16 so the loop
becomes ((iId == 0) ? 31 : 16) to prevent iterating over the Kick bit when iId
!= 0.

In `@src/mumble/MainWindow.cpp`:
- Line 2631: The current ternary-style condition on qaUserTextMessage enables
the control unconditionally for servers with version < 1.7.0 or UNKNOWN; change
it so legacy/unknown servers still go through the appropriate permission check
instead of being auto-enabled. Update the expression around
qaUserTextMessage->setEnabled to first ensure Global::get().sh exists, then if
Global::get().sh->m_version >= Version::fromComponents(1,7,0) use the existing
!(p & (ChanACL::Write | ChanACL::PrivateMessage)) check, otherwise
(legacy/unknown branch) perform the legacy permission check (i.e. evaluate the
same or correct legacy ACL bits) so the widget is only enabled when permissions
allow.

In `@src/murmur/Messages.cpp`:
- Around line 1741-1744: The DM permission check in Messages.cpp (the
ChanACL::hasPermission call that currently requires ChanACL::PrivateMessage or
ChanACL::Write for uSource on u->cChannel) can block legacy users because ACL
defaults still grant ChanACL::TextMessage; add a compatibility fallback so that
if neither PrivateMessage nor Write is granted you also allow the action when
ChanACL::hasPermission(uSource, u->cChannel, ChanACL::TextMessage, &acCache) is
true. Modify the conditional around ChanACL::hasPermission in the function
handling private messages to include a third check for ChanACL::TextMessage (or
invert the test: deny only if none of PrivateMessage, Write, or TextMessage are
granted) so upgrades remain backward-compatible until ACL defaults/migration are
changed.

---

Nitpick comments:
In `@src/ACL.cpp`:
- Around line 374-375: permName() returns "PrivateMessage" for the
PrivateMessage enum case which is inconsistent with other human-readable
permission labels; update the PrivateMessage branch in ACL.cpp to return a
spaced, user-facing label (e.g., return tr("Private Message")) so it matches the
style used by the other permission cases and the ACL UI.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4e3b4d90-9573-4bb4-9397-bb731520f715

📥 Commits

Reviewing files that changed from the base of the PR and between c73aee6 and 72ec7fc.

⛔ Files ignored due to path filters (2)

• src/mumble/mumble_de.ts is excluded by !**/*.ts
• src/mumble/mumble_en_GB.ts is excluded by !**/*.ts

📒 Files selected for processing (5)

• src/ACL.cpp
• src/ACL.h
• src/mumble/ACLEditor.cpp
• src/mumble/MainWindow.cpp
• src/murmur/Messages.cpp

[lunyav]

Tho I do feel unsure about TextMessage as a overall fallback, that doesnt seem to fulfill the requirement in the Issue that these two things can be privileged seperately.
I think Id prefer an upgrade, is there a scheme how upgrades from e.g. 1.6 => 1.7 are done with murmur?

[Hartmnt]

Tho I do feel unsure about TextMessage as a overall fallback, that doesnt seem to fulfill the requirement in the Issue that these two things can be privileged seperately.

Not sure what you mean, but the client knows the version the server is running on. So it can decide to apply the legacy TextMessage permission or the new PrivateMessage permission based on the server version.

I think Id prefer an upgrade, is there a scheme how upgrades from e.g. 1.6 => 1.7 are done with murmur?

This is a valid concern I did not think about on the server side though. We have automatic database migrations, and I think that you are right and we would have to introduce a migration path which enables PrivateMessage for all ACLs which currently have TextMessage enabled. I think we never had the case before that an existing ACL was split into two.

[Hartmnt]

Please squash all of the commits except the translation commit into one. You can most easily do so by looking up "git interactive rebase" and then you will have to force-push your branch. Don't worry force-pushing is the correct thing to do here.

Since we have to change the translatable string PrivateMessage into Private Message, you have to drop (and then later regenerate) the translation commit again (can also be done via interactive rebase)

I'd recommend adding the translation commit when everything else is done and resolved. After all we might want to change something still.

[lunyav]

Tho I do feel unsure about TextMessage as a overall fallback, that doesnt seem to fulfill the requirement in the Issue that these two things can be privileged seperately.

Not sure what you mean, but the client knows the version the server is running on. So it can decide to apply the legacy TextMessage permission or the new PrivateMessage permission based on the server version.

I think Id prefer an upgrade, is there a scheme how upgrades from e.g. 1.6 => 1.7 are done with murmur?

This is a valid concern I did not think about on the server side though. We have automatic database migrations, and I think that you are right and we would have to introduce a migration path which enables PrivateMessage for all ACLs which currently have TextMessage enabled. I think we never had the case before that an existing ACL was split into two.

I was talking in the first part about the server side fallback which I dont think is a good solution.

[Hartmnt]

@lunyav Yep, so I think the solution on the server side is (and please @Krzmbrzl correct me if I am wrong) we need to increase the database schema and migrate existing channel ACLs. Those ACLs which allow TextMessage should allow both TextMessage and PrivateMessage after the migration. Those ACLs which don't allow TextMessage should allow neither after the migration.

The migration happens once after a Mumble server operator updates to Mumble 1.7. After that they are free to assign and unassign the ACLs individually.

To do this, you first increase the database schema version at src/murmur/database/ServerDatabase.h then you'd want to go into src/murmur/database/ACLTable.cpp and modify the void ACLTable::migrate(unsigned int fromSchemaVersion, unsigned int toSchemaVersion) function. You'd then add a test case in src/tests/TestDatabase/server/table_data/ to make sure the migration works and does not break in the future.

You can also refer to the WIP documentation for this at #6928

[lunyav]

@Hartmnt How do I run the tests locally? Or should I just push and let the CI do that?

[Hartmnt]

@Hartmnt How do I run the tests locally? Or should I just push and let the CI do that?

push/force push is fine.

[Krzmbrzl]

How do I run the tests locally?

You can use ctest for that. See https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html#testing-using-ctest

The tests with Sqlite don't require any DB setup and the other backends are not tested unless explicitly enabled. See https://github.com/mumble-voip/mumble/blob/master/src/tests/TestDatabase/README.md

[lunyav]

I dont feel like I understand why the test fails. Like why does it say that four rows were removed when I only did update the table? From my perspective this looks fine now.
After a bit of debugging it actually seems like the table is empty after migration, not sure why.