FEAT(client): Allow not saving passwords #6820
Conversation
maxer137
force-pushed
the
save-password
branch
2 times, most recently
from
ad1cac3 to
5b6d16b
Compare
maxer137
left a comment
maxer137
left a comment
There was a problem hiding this comment.
I had quite the odd look on my face when it turned out I couldn’t see my comments when checking on mobile.
Seems I’ve been a victim of the GitHub [Pending] UI
Yeah, I have been there before too. |
Hartmnt
left a comment
Hartmnt
left a comment
There was a problem hiding this comment.
Ok, I finally had time to compile and try this. While your change does what it advertises, I ran into two usability issues:
-
If I select the "Suppress certificate and password storage" setting (
bSuppressIdentity), it still shows your checkbox (in a checked state for that matter) despite the password not being saved either way. We should hide or disable the checkbox, if that setting is true. -
If I connect to a server which is not stored as favorite, it does also not save the password either way. That is not a problem with your code specifically, it looks like it was never intended to store information of non-favorites. See:
it says "UPDATE" andmumble/src/mumble/Database.cpp
Lines 721 to 730 in 41e381d
serversonly contain favorites.
Anyway, we should also hide or disable the checkbox in those cases.
Alternatively, we could |
|
I'm wondering what is the best way to implement the password prompt. I could implement a function in MainWindow that, depending on the setting, either prompts the new PasswordDialog screen or simply the password prompt without checkbox. Additionally. I think it's important to explain to the user that the checkbox is missing because they enabled the Suppress Identity setting. Just in case. |
I would suggest adding this as a possible option (or multiple options) for PasswordDialog, now that we have a custom class for that.
That's why I lean more towards disabling instead of hiding the checkbox. You could add a label with "disabled by global configuration" or something similar. |
I would ensure that we specifically mention which setting disables the checkbox. So that if a user is wondering why the checkbox is disabled, they can specifically target that setting. In the case they turned it on accidentally.
Shall I get the fetching of the setting inside PasswordDialog or MainWindow? |
I think the reasonable thing to do is to fetch the (or later all) settings which may prevent saving the password in the PasswordDialog |
maxer137
force-pushed
the
save-password
branch
2 times, most recently
from
76113e3 to
aa7e322
Compare
Hartmnt
left a comment
Hartmnt
left a comment
There was a problem hiding this comment.
Sorry, this iteration took a bit longer to review. I still try to look into your merge request whenever I have a bit of time.
src/mumble/PasswordDialog.cpp
Outdated
| dialog.qcbSavePassword->setChecked(save ? *save : false); | ||
| } else { | ||
| dialog.qcbSavePassword->setEnabled(false); | ||
| QString new_tooltip = QObject::tr("Password can't be saved: ") + toString(checkResult.reason); |
There was a problem hiding this comment.
| QString new_tooltip = QObject::tr("Password can't be saved: ") + toString(checkResult.reason); | |
| QString new_tooltip = QObject::tr("Password can't be saved: %1").arg(toString(checkResult.reason)); |
That way translators can change the order of the sentence if they need to.
maxer137
force-pushed
the
save-password
branch
from
aa7e322 to
69085d8
Compare
|
Could you please rebase this against master? This branch is missing a fix for a crash from a month ago and I can't test it again 😅 |
I am going to have to spend some more time analyzing the new FailedConnectionDialog to understand if this is still valid. It might take a moment to test if the new version still works properly. |
…enabled This change was made so the future commit can optionally save passwords.
When asked for a password, the user has the option to save or not save the password to the device. Implements mumble-voip#1087
maxer137
force-pushed
the
save-password
branch
from
69085d8 to
4164b3f
Compare
Walkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dialog as FailedConnectionDialog
participant Global as Global Settings
participant DB as Database
User->>Dialog: Open after connection failure
Dialog->>Global: Read bSuppressIdentity
Global-->>Dialog: boolean
Note over Dialog: qcbSavePassword.setChecked(!bSuppressIdentity)
User->>Dialog: Enter/confirm password<br/>Toggle "Save settings on this device"
User->>Dialog: Click Reconnect
alt "Save settings on this device" is checked
Dialog->>DB: setPassword(host, port, user, password)
Note right of DB: UPDATE OR INSERT into servers
else not checked
Dialog-->>DB: Skip password persistence
end
Dialog-->>User: Proceed with reconnect flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mumble/Database.cpp(1 hunks)src/mumble/NetworkConfig.ui(1 hunks)src/mumble/widgets/FailedConnectionDialog.cpp(2 hunks)src/mumble/widgets/FailedConnectionDialog.ui(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: PR (Docs)
- GitHub Check: PR (Translations)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (Linux)
- GitHub Check: PR (macOS)
- GitHub Check: CodeQL-Build (ubuntu-latest)
- GitHub Check: build (ubuntu-22.04, shared, 64bit)
- GitHub Check: build (ubuntu-24.04, shared, 64bit)
- GitHub Check: freebsd
- GitHub Check: pr-checks (ubuntu-latest)
🔇 Additional comments (5)
src/mumble/widgets/FailedConnectionDialog.cpp (2)
27-27: LGTM!The checkbox initialization logic correctly sets the default state based on the inverse of the identity suppression setting, providing good UX by defaulting to enabled when identity is not suppressed.
74-76: LGTM!The conditional password saving logic correctly implements the new user-controlled behavior, allowing users to explicitly choose whether to save their credentials.
src/mumble/widgets/FailedConnectionDialog.ui (2)
9-9: Minor width increase to accommodate new checkbox.The dialog width increase from 409 to 424 pixels provides adequate space for the new save password checkbox.
259-265: LGTM!The new Save Password checkbox is appropriately positioned before the Reconnect button, providing a clear and intuitive user flow for the password saving decision.
src/mumble/Database.cpp (1)
721-729: Add a UNIQUE index on (hostname,port,username) and use proper SQLite UPSERT (ON CONFLICT)
- Add schema index in Database ctor (alongside other indexes):
execQueryAndLogFailure(query, QLatin1String( "CREATE UNIQUE INDEX IF NOT EXISTS `servers_host_port_user` " "ON `servers`(`hostname`,`port`,`username`)"));
- Replace the current UPDATE/INSERT in Database::setPassword (src/mumble/Database.cpp) with a proper INSERT...ON CONFLICT and bind values in column order:
query.prepare(QLatin1String( "INSERT INTO `servers` (`hostname`,`port`,`username`,`password`) VALUES (?,?,?,?) " "ON CONFLICT(`hostname`,`port`,`username`) DO UPDATE SET `password`=excluded.`password`")); query.addBindValue(hostname); query.addBindValue(port); query.addBindValue(uname); query.addBindValue(pw);Quick repo search did not find explicit favorites accessors (getFavorites/FavoriteServer); verify that enabling UPSERT won’t create unnamed rows that the UI would expose as favorites.
| query.prepare(QLatin1String( | ||
| "UPDATE OR INSERT `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalid SQLite syntax: “UPDATE OR INSERT” is not supported (QSQLITE).
This will fail at runtime. If we keep the “favorites-only in servers” invariant, switch back to UPDATE-only and let the caller decide whether saving is available.
Minimal fix:
- query.prepare(QLatin1String(
- "UPDATE OR INSERT `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?"));
+ query.prepare(QLatin1String(
+ "UPDATE `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?"));Optional: detect “no row updated” and surface it (log or change signature to return bool):
query.addBindValue(uname);
- execQueryAndLogFailure(query);
+ if (execQueryAndLogFailure(query) && query.numRowsAffected() == 0) {
+ qWarning() << "Database::setPassword: no matching server to update (hostname/port/username).";
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query.prepare(QLatin1String( | |
| "UPDATE OR INSERT `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?")); | |
| query.prepare(QLatin1String( | |
| "UPDATE `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?")); |
🤖 Prompt for AI Agents
In src/mumble/Database.cpp around lines 723-724, the SQL uses unsupported
"UPDATE OR INSERT" for QSQLITE; replace it with a plain UPDATE statement so the
call only updates existing rows (preserving the "favorites-only in servers"
invariant) and remove any INSERT behavior; additionally, consider detecting when
no rows were updated (e.g. check query.numRowsAffected() or the execute result)
and either log that fact or change the function signature/return to surface a
bool success so the caller can decide whether to perform an insert.
| </property> | ||
| <property name="whatsThis"> | ||
| <string><b>This will suppress identity information from the client.</b><p>The client will not identify itself with a certificate, even if defined, and will not cache passwords for connections. This is primarily a test-option and is not saved.</p></string> | ||
| <string><b>This will suppress identity information from the client.</b><p>The client will not identify itself with a certificate, even if defined. This is primarily a test-option and is not saved.</p></string> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align wording with new behavior; remove password mention and tighten phrasing.
Tooltip/label should no longer imply that this option affects password saving. Also prefer “test option” (no hyphen).
Apply:
- <property name="toolTip">
- <string>Don't send certificate to server and don't save passwords. (Not saved).</string>
- </property>
+ <property name="toolTip">
+ <string>Do not send a client certificate to the server. This is primarily a test option and is not saved.</string>
+ </property>
@@
- <string><b>This will suppress identity information from the client.</b><p>The client will not identify itself with a certificate, even if defined. This is primarily a test-option and is not saved.</p></string>
+ <string><b>This suppresses client identity information.</b><p>The client will not identify itself with a certificate, even if defined. This is primarily a test option and is not saved.</p></string>
@@
- <string>Suppress certificate information</string>
+ <string>Suppress client certificate</string>Also applies to: 90-90
🤖 Prompt for AI Agents
In src/mumble/NetworkConfig.ui around lines 87 and 90, the tooltip text still
mentions password saving and uses “test-option”; update the wording to remove
any reference to passwords, tighten the phrasing, and use “test option” (no
hyphen). Replace the current string with a concise message stating that identity
information is suppressed, the client will not present a certificate for
identification, and that this is a test option which is not saved.
|
I really do not like the coderabbitai service. Is there a way to disable it? |
I don't think so 🥲 I assume you already know, but the current iteration of this PR fails with the SQL error |
No - other than the entire handling of failed but potentially "recoverable" connection attempts is now done in the new dialog. |
It's enabled on purpose and there is no way for users to opt-out from it. It only auto-reviews non-draft PRs once though. While I agree that it currently still creates more comments than strictly necessary, I have found its actual reviews to be relatively good. Far from perfect but it has identified several issues that would have required significant time investments of a human reviewer or which have even been missed entirely. For instance, the |
3 participants
When asked for a password, the user has the option to save or not save the password to the device.
Implements #1087
Checks