ACLEditor: warn when adding ACL for unregistered user#7171
ACLEditor: warn when adding ACL for unregistered user#7171prahladp03 wants to merge 1 commit intomumble-voip:masterfrom
Conversation
WalkthroughThis pull request adds user registration validation to the ACL editor. Changes in 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/mumble/ACLEditor.cpp (1)
422-435: Modal dialog inside network response handler.
QMessageBox::warningis modal and spins a nested event loop while aMumbleProto::QueryUsersresponse is being processed. FurtherServerHandlermessages may be dispatched re-entrantly into this editor while the dialog is up. Consider deferring the warning withQTimer::singleShot(0, ...)or using a non-modal notification (Log::log) to avoid re-entrancy surprises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mumble/ACLEditor.cpp` around lines 422 - 435, The loop over qlACLs in ACLEditor.cpp calls the modal QMessageBox::warning when an unregistered ChanACL (iUserId < -1 and !bInherited) is found, which can cause re-entrant ServerHandler processing; change this to defer the warning or make it non-modal: instead of calling QMessageBox::warning inline in the loop, capture the computed message (using qhNameCache.value(acl->iUserId)) and either call QTimer::singleShot(0, this, [this, msg]() { QMessageBox::warning(this, tr("Mumble — Unregistered user"), msg); }) or send the text to the non-modal logging facility (Log::log) so the warning is shown asynchronously and avoids nested event loops; update the code around the ChanACL iteration and any helper that builds the tr(...) message to use the chosen approach.
🤖 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`:
- Around line 424-432: The warning shows a lowercased name because
ACLEditor::id() stores names with uname.toLower() into qhNameCache; update the
flow to preserve original casing and use that for the dialog: either change
ACLEditor::id() to store a struct/tuple or separate map that keeps the
original-cased name alongside the lowercase key, or when building the message
here prefer the original name source (for example mqu.names(i) from the loop
that populated qhNameCache or the original key from qhNameWait) instead of
qhNameCache.value(...); ensure qhNameCache lookups still work for
case-insensitive matching but use the preserved original string for display in
QMessageBox::warning.
- Around line 420-435: The loop in returnQuery warns for every unresolved ACL in
qlACLs on every QueryUsers response, causing duplicate warnings; modify
returnQuery to only warn for user IDs present in this response's batch (use
mqu.names() or the response-specific IDs) instead of iterating all qlACLs, or
alternatively keep a QSet<int> qsWarnedUnregistered (declare in ACLEditor.h) to
skip already-warned iUserId values; use qhNameCache.value(iUserId) only for IDs
in the current mqu.names() set (or check qsWarnedUnregistered before showing
QMessageBox and add the id after warning) so each unregistered user warns
exactly once per session.
In `@src/tests/TestACLUserRegistration/TestACLUserRegistration.cpp`:
- Around line 18-68: The tests in TestACLUserRegistration are tautological and
use the wrong predicate (they assert userId < 0 instead of production logic
which treats iUserId == -1 as a non-temporary group entry); replace these
meaningless local checks by exercising the real predicate: extract the
production predicate into a small testable helper (e.g.
ACLEditorDetail::isTemporaryUserId(int) or
aclEntryNeedsRegistrationWarning(const ChanACL&)) that implements iUserId < -1
(refer to ACLEditor.cpp:423 and the accept() loop at line 329 for behavior),
then update TestACLUserRegistration to include that header and assert expected
results (userId == -1 -> false, userId <= -2 -> true, 0 and positive -> false)
or alternatively remove these unit tests and add an integration test that drives
ACLEditor::returnQuery with a fake QueryUsers message to validate real behavior.
- Around line 9-16: The header comment is incorrect: update it to state that
iUserId == -1 denotes a group ACL entry (not an unregistered user) and that
unresolved/unregistered users use iUnknown values starting at -2; adjust the
explanatory text to reference the actual symbols and code paths (mention
ACLEditor.cpp and the returnQuery method/logic where the warning is emitted, and
the User::iId / iUnknown convention), and remove or replace the outdated "See:
on_qcbACLUser_textActivated" reference with a pointer to returnQuery and the
relevant lines in ACLEditor.cpp that set iUnknown.
---
Nitpick comments:
In `@src/mumble/ACLEditor.cpp`:
- Around line 422-435: The loop over qlACLs in ACLEditor.cpp calls the modal
QMessageBox::warning when an unregistered ChanACL (iUserId < -1 and !bInherited)
is found, which can cause re-entrant ServerHandler processing; change this to
defer the warning or make it non-modal: instead of calling QMessageBox::warning
inline in the loop, capture the computed message (using
qhNameCache.value(acl->iUserId)) and either call QTimer::singleShot(0, this,
[this, msg]() { QMessageBox::warning(this, tr("Mumble — Unregistered user"),
msg); }) or send the text to the non-modal logging facility (Log::log) so the
warning is shown asynchronously and avoids nested event loops; update the code
around the ChanACL iteration and any helper that builds the tr(...) message to
use the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04db5dd2-1347-4f32-a5af-72ecc2b8ffbb
📒 Files selected for processing (4)
src/mumble/ACLEditor.cppsrc/tests/CMakeLists.txtsrc/tests/TestACLUserRegistration/CMakeLists.txtsrc/tests/TestACLUserRegistration/TestACLUserRegistration.cpp
| // Warn if any ACL entry still has a negative user ID after server | ||
| // response, meaning the user is not registered on this server. | ||
| for (ChanACL *acl : qlACLs) { | ||
| if (!acl->bInherited && acl->iUserId < -1) { | ||
| const QString name = qhNameCache.value(acl->iUserId); | ||
| if (!name.isEmpty()) { | ||
| QMessageBox::warning( | ||
| this, | ||
| tr("Mumble \u2014 Unregistered user"), | ||
| tr("The user \"%1\" is not registered on this server. " | ||
| "ACL entries for unregistered users are discarded by the server and will have no effect. " | ||
| "Please register this user before adding them to an ACL.") | ||
| .arg(name)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Warning will fire repeatedly for the same user on each returnQuery.
returnQuery is invoked for every QueryUsers response. The newly added loop iterates over all qlACLs and warns for any still-unregistered entry, not just the users resolved in this particular response. Consequences:
- Add two unknown users A and B. The server sends two separate
QueryUsersresponses. When A's response arrives (A unresolved), the loop warns for both A and B. When B's response arrives, it warns for B again — and for A again. - More generally, any subsequent user lookup (e.g. autocomplete, adding another ACL entry) will re-trigger warnings for every pre-existing unregistered ACL entry.
Either (a) only warn for user IDs that correspond to names in this response's mqu.names() batch, or (b) track which tids have already been warned about (e.g. a QSet<int> qsWarnedUnregistered), or (c) remove the entry from qhNameCache after warning so the !name.isEmpty() guard trips. Option (a) is probably cleanest:
🔧 Suggested fix — warn only within the current response batch
- // Warn if any ACL entry still has a negative user ID after server
- // response, meaning the user is not registered on this server.
- for (ChanACL *acl : qlACLs) {
- if (!acl->bInherited && acl->iUserId < -1) {
- const QString name = qhNameCache.value(acl->iUserId);
- if (!name.isEmpty()) {
- QMessageBox::warning(
- this,
- tr("Mumble \u2014 Unregistered user"),
- tr("The user \"%1\" is not registered on this server. "
- "ACL entries for unregistered users are discarded by the server and will have no effect. "
- "Please register this user before adding them to an ACL.")
- .arg(name));
- }
- }
- }
+ // Warn once per unregistered user that was just looked up. An unregistered
+ // user is one whose name was queried (qhNameWait still holds the entry)
+ // and for which the server did not return a registration id in this batch.
+ for (int i = 0; i < mqu.names_size(); ++i) {
+ const QString lname = u8(mqu.names(i)).toLower();
+ if (!qhNameWait.contains(lname) || qsWarnedUnregistered.contains(lname))
+ continue;
+ // Still pending => server did not resolve it in this response.
+ qsWarnedUnregistered.insert(lname);
+ QMessageBox::warning(
+ this,
+ tr("Mumble — Unregistered user"),
+ tr("The user \"%1\" is not registered on this server. "
+ "ACL entries for unregistered users are discarded by the server and will have no effect. "
+ "Please register this user before adding them to an ACL.")
+ .arg(u8(mqu.names(i))));
+ }(Requires adding QSet<QString> qsWarnedUnregistered; in ACLEditor.h.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/ACLEditor.cpp` around lines 420 - 435, The loop in returnQuery
warns for every unresolved ACL in qlACLs on every QueryUsers response, causing
duplicate warnings; modify returnQuery to only warn for user IDs present in this
response's batch (use mqu.names() or the response-specific IDs) instead of
iterating all qlACLs, or alternatively keep a QSet<int> qsWarnedUnregistered
(declare in ACLEditor.h) to skip already-warned iUserId values; use
qhNameCache.value(iUserId) only for IDs in the current mqu.names() set (or check
qsWarnedUnregistered before showing QMessageBox and add the id after warning) so
each unregistered user warns exactly once per session.
| const QString name = qhNameCache.value(acl->iUserId); | ||
| if (!name.isEmpty()) { | ||
| QMessageBox::warning( | ||
| this, | ||
| tr("Mumble \u2014 Unregistered user"), | ||
| tr("The user \"%1\" is not registered on this server. " | ||
| "ACL entries for unregistered users are discarded by the server and will have no effect. " | ||
| "Please register this user before adding them to an ACL.") | ||
| .arg(name)); |
There was a problem hiding this comment.
Warning displays lowercased user name.
qhNameCache.value(acl->iUserId) returns the name inserted by ACLEditor::id() at line 386, which stores name = uname.toLower(). So the dialog shows e.g. "alice" even if the admin typed "Alice". Prefer using the originally-typed name (e.g. from mqu.names(i) in the loop above, or qhNameWait's key's original casing — which is currently lost; you could carry the original casing alongside).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/ACLEditor.cpp` around lines 424 - 432, The warning shows a
lowercased name because ACLEditor::id() stores names with uname.toLower() into
qhNameCache; update the flow to preserve original casing and use that for the
dialog: either change ACLEditor::id() to store a struct/tuple or separate map
that keeps the original-cased name alongside the lowercase key, or when building
the message here prefer the original name source (for example mqu.names(i) from
the loop that populated qhNameCache or the original key from qhNameWait) instead
of qhNameCache.value(...); ensure qhNameCache lookups still work for
case-insensitive matching but use the preserved original string for display in
QMessageBox::warning.
| // The ACL editor determines whether a user is registered by checking | ||
| // whether their user ID is non-negative. A user ID of -1 indicates an | ||
| // unregistered (guest) user, while a user ID >= 0 indicates a registered | ||
| // user. This is the same logic used in ACLEditor.cpp to decide whether | ||
| // to show a warning when an administrator adds a user to an ACL. | ||
|
|
||
| // See: src/mumble/ACLEditor.cpp (on_qcbACLUser_textActivated) | ||
| // See: src/User.h (iId field) |
There was a problem hiding this comment.
Comment contradicts the implementation.
The header comment says "A user ID of -1 indicates an unregistered (guest) user, while a user ID >= 0 indicates a registered user." In the actual ACLEditor code, iUserId == -1 means the ACL is a group entry (see ACLEditor.cpp:188, line 329, line 492), and unregistered/unresolved users are assigned iUnknown values starting at -2 (line 231, 384). Please update the comment and the referenced "See: on_qcbACLUser_textActivated" (the new warning lives in returnQuery, not that slot).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/TestACLUserRegistration/TestACLUserRegistration.cpp` around lines 9
- 16, The header comment is incorrect: update it to state that iUserId == -1
denotes a group ACL entry (not an unregistered user) and that
unresolved/unregistered users use iUnknown values starting at -2; adjust the
explanatory text to reference the actual symbols and code paths (mention
ACLEditor.cpp and the returnQuery method/logic where the warning is emitted, and
the User::iId / iUnknown convention), and remove or replace the outdated "See:
on_qcbACLUser_textActivated" reference with a pointer to returnQuery and the
relevant lines in ACLEditor.cpp that set iUnknown.
| class TestACLUserRegistration : public QObject { | ||
| Q_OBJECT | ||
| private slots: | ||
| void registeredUser_hasNonNegativeId() { | ||
| // A registered user always has iId >= 0 | ||
| int userId = 42; | ||
| QVERIFY(userId >= 0); | ||
| } | ||
|
|
||
| void unregisteredUser_hasNegativeId() { | ||
| // An unregistered (guest) user has iId == -1 | ||
| int userId = -1; | ||
| QVERIFY(userId < 0); | ||
| } | ||
|
|
||
| void unknownUser_hasNegativeId() { | ||
| // A user whose name could not be resolved also gets | ||
| // a temporary negative ID (iUnknown, starting at -2) | ||
| int userId = -2; | ||
| QVERIFY(userId < 0); | ||
| } | ||
|
|
||
| void registrationCheck_correctlyIdentifiesRegistered() { | ||
| // The warning should not fire for registered users | ||
| int userId = 100; | ||
| bool shouldWarn = (userId < 0); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_correctlyIdentifiesUnregistered() { | ||
| // The warning should fire for unregistered users | ||
| int userId = -1; | ||
| bool shouldWarn = (userId < 0); | ||
| QVERIFY(shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_boundaryAtZero() { | ||
| // User ID of exactly 0 is valid (registered) | ||
| int userId = 0; | ||
| bool shouldWarn = (userId < 0); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_temporaryIds() { | ||
| // Temporary IDs assigned during name lookup are also negative | ||
| for (int tempId = -2; tempId >= -10; tempId--) { | ||
| bool shouldWarn = (tempId < 0); | ||
| QVERIFY(shouldWarn); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Tests do not exercise any production code — and use a different predicate than production.
Two problems:
-
Tautological: Each test declares a local
int userIdand asserts properties of< 0/>= 0. No symbol fromACLEditoror any other source file is referenced. These tests would still pass if the ACL warning logic were deleted or inverted. They verify C++ integer comparison, not Mumble behavior. -
Wrong predicate (more important): The production warning at
ACLEditor.cpp:423usesacl->iUserId < -1, becauseiUserId == -1is the sentinel for "group ACL, not a user ACL" (see theaccept()loop at line 329 that also uses< -1, and the manyiUserId = -1assignments throughout the file). The tests instead assertshouldWarn = (userId < 0):registrationCheck_correctlyIdentifiesUnregisteredsetsuserId = -1and assertsshouldWarnis true. But in production, an entry withiUserId == -1is a group entry and must not warn — and indeed does not.unregisteredUser_hasNegativeId's comment states "An unregistered (guest) user has iId == -1", but perDBWrapper.cpp:460andACLEditor.cpp's usage,-1is "no user / group entry"; unregistered/unresolved names get IDs<= -2viaiUnknown.
Because these tests don't import/call the real check, they silently diverge from the implementation. Consider either (a) extracting the predicate into a small free function (e.g. bool isTemporaryUserId(int) or bool aclEntryNeedsRegistrationWarning(const ChanACL&)) and testing that, or (b) dropping these unit tests in favor of a proper ACLEditor integration test that drives returnQuery with a fake QueryUsers message.
Example: extract predicate for testability
// In ACLEditor.h (or a small helper header)
namespace ACLEditorDetail {
inline bool isTemporaryUserId(int userId) { return userId < -1; }
}Then the tests can #include it and actually exercise production logic, with userId == -1 expected to return false.
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If slots is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/TestACLUserRegistration/TestACLUserRegistration.cpp` around lines
18 - 68, The tests in TestACLUserRegistration are tautological and use the wrong
predicate (they assert userId < 0 instead of production logic which treats
iUserId == -1 as a non-temporary group entry); replace these meaningless local
checks by exercising the real predicate: extract the production predicate into a
small testable helper (e.g. ACLEditorDetail::isTemporaryUserId(int) or
aclEntryNeedsRegistrationWarning(const ChanACL&)) that implements iUserId < -1
(refer to ACLEditor.cpp:423 and the accept() loop at line 329 for behavior),
then update TestACLUserRegistration to include that header and assert expected
results (userId == -1 -> false, userId <= -2 -> true, 0 and positive -> false)
or alternatively remove these unit tests and add an integration test that drives
ACLEditor::returnQuery with a fake QueryUsers message to validate real behavior.
prahladp03
force-pushed
the
acl-warn-unregistered-user
branch
2 times, most recently
from
3820496 to
a72a503
Compare
ACLs can only be applied to registered users. If an unregistered user is selected in the ACL editor, the server silently discards the entry, which is confusing for administrators. This change adds a warning dialog in returnQuery() that fires after the server has responded to a user lookup. If the user ID remains negative after the server response, the user is not registered and a warning is shown explaining that the ACL entry will have no effect. Also adds unit tests for the registration detection logic used by the warning (TestACLUserRegistration). Fixes mumble-voip#6961
prahladp03
force-pushed
the
acl-warn-unregistered-user
branch
from
a72a503 to
afb5a42
Compare
|
I've addressed the following feedback:
|
There was a problem hiding this comment.
I will give you the benefit of the doubt here and say that you are a total beginner to software development and trying your best to learn and improve. Using a LLM is, in my humble opinion, not a good way to do so. But to each their own I suppose.
The problem with what you have submitted here is that the quality is not even close to decent, in a way that makes it hard to not be offended by having to read it.
Your proposed solution does not take any input in the original issue #6961 into account. It barely does anything, but at least it prompts the user in a very annoying way. On top of that you did not follow our commit guidelines, you did not take a look at the repo to find out how others are contributing code, you also did not read or care about the existing LLM code reviews.
So, I hope you will benefit from constructive criticism and me writing this wall of text. I don't know how to proceed with this PR. I will convert this to a draft and see if you still want to work on this, but I guess at this point you could also start over. IDK
| qsWarnedUnregistered.insert(lname); | ||
| QMessageBox::warning( | ||
| this, | ||
| tr("Mumble \u2014 Unregistered user"), |
There was a problem hiding this comment.
This has to be one of the funniest lines of code ever to be submitted to any project ever
| // Copyright The Mumble Developers. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license | ||
| // that can be found in the LICENSE file at the root of the | ||
| // Mumble source tree or at <https://www.mumble.info/LICENSE>. | ||
|
|
||
| #include <QObject> | ||
| #include <QTest> | ||
|
|
||
| // The ACL editor determines whether a user lookup was unresolved by checking | ||
| // whether their user ID is less than -1. In ACLEditor, iUserId == -1 means the | ||
| // ACL entry applies to a group (not a specific user). Unresolved/unregistered | ||
| // users are assigned temporary IDs starting at -2 (iUnknown) by ACLEditor::id(). | ||
| // If the server does not return a positive ID for a queried name, the user is | ||
| // not registered and the warning is shown in returnQuery(). | ||
| // | ||
| // See: src/mumble/ACLEditor.cpp (returnQuery, ACLEditor::id) | ||
| // See: src/User.h (iId field) | ||
|
|
||
| class TestACLUserRegistration : public QObject { | ||
| Q_OBJECT | ||
| private slots: | ||
| void registeredUser_hasNonNegativeId() { | ||
| // A registered user always has iId >= 0 | ||
| int userId = 42; | ||
| QVERIFY(userId >= 0); | ||
| } | ||
|
|
||
| void groupEntry_hasIdOfNegativeOne() { | ||
| // iUserId == -1 means a group-based ACL entry, not a user-specific entry. | ||
| // This should NOT trigger the warning (predicate is < -1, not < 0) | ||
| int userId = -1; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void unknownUser_hasNegativeId() { | ||
| // A user whose name could not be resolved gets a temporary negative ID | ||
| // (iUnknown, starting at -2). These SHOULD trigger the warning. | ||
| int userId = -2; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_correctlyIdentifiesRegistered() { | ||
| // The warning should NOT fire for registered users (iId >= 0) | ||
| int userId = 100; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_correctlyIdentifiesUnregistered() { | ||
| // The warning fires for temporary IDs (< -1), not for -1 (group entry) | ||
| int userId = -2; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_boundaryAtZero() { | ||
| // User ID of exactly 0 is valid (registered), should not warn | ||
| int userId = 0; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_groupEntrySentinel() { | ||
| // User ID of -1 is the group ACL sentinel, should NOT warn | ||
| int userId = -1; | ||
| bool shouldWarn = (userId < -1); | ||
| QVERIFY(!shouldWarn); | ||
| } | ||
|
|
||
| void registrationCheck_temporaryIds() { | ||
| // Temporary IDs assigned during name lookup (iUnknown starting at -2) | ||
| // should trigger the warning since they are < -1 | ||
| for (int tempId = -2; tempId >= -10; tempId--) { | ||
| bool shouldWarn = (tempId < -1); | ||
| QVERIFY(shouldWarn); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| QTEST_MAIN(TestACLUserRegistration) | ||
| #include "TestACLUserRegistration.moc" |
There was a problem hiding this comment.
Ok so I will try to keep this as good faith as possible. Let's use this as an exercise: Without asking a LLM, what do you actually test here? What exactly have you achieved if you prove
int userId = 42;
QVERIFY(userId >= 0);
In software development we want to use tests to verify that code does what it should do. Ask yourself "What possible mistake could a programmer make in the original ACLEditor code that would make this test fail". If you come up with the answer to this without asking a LLM, you will have learned something.
There was a problem hiding this comment.
You have either asked a LLM to generate the translation files or squashed the translation commits into your feature commit. Both is unacceptable.
| QHash< int, QString > qhNameCache; | ||
| QHash< QString, int > qhIDCache; | ||
| QHash< QString, int > qhNameWait; | ||
| QSet< QString > qsWarnedUnregistered; |
There was a problem hiding this comment.
Use std containers wherever possible in newly introduced code
| // Warn once per unregistered user that was just looked up. An unregistered | ||
| // user is one whose name was queried but for which the server did not return | ||
| // a valid registration ID in this response batch. |
There was a problem hiding this comment.
The premise of this is bad: I don't want to read a message box for all unregistered users. A single user at a time would be ok. Imaging having to click through a dozen message boxes before being able to fix the problem.
Even worse, it looks like the ACLEditor might query users one at the time. Each time we receive an answer all unregistered ones produce message boxes.
2 participants
ACLs can only be applied to registered users. If an unregistered user is selected in the ACL editor, the server silently discards the entry, which is confusing for administrators.
This change adds a warning dialog in returnQuery() that fires after the server responds to a user lookup. If the user ID remains negative after the server response, the user is not registered and a warning is shown explaining that the ACL entry will have no effect and that the user needs to be registered first.
The warning is placed in returnQuery() rather than at selection time to avoid false positives during async name resolution for registered users who are currently offline.
Also adds unit tests (TestACLUserRegistration) covering the registration detection logic.
Fixes #6961