FEAT(client): Allow not saving passwords#6820
FEAT(client): Allow not saving passwords#6820maxer137 wants to merge 2 commits intomumble-voip:masterfrom
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. |
maxer137
force-pushed
the
save-password
branch
from
69085d8 to
4164b3f
Compare
WalkthroughThe changes modify certificate and password storage handling across the connection configuration interface. The NetworkConfig.ui text was updated to specify "certificate information" instead of "certificate and password storage." A new "Save settings on this device" checkbox was added to FailedConnectionDialog with accompanying UI layout adjustments. The password persistence logic was revised to depend on whether the server is marked as a favorite and the checkbox is checked, replacing the prior reliance on a global identity suppression setting. The checkbox becomes disabled for non-favorited servers with explanatory messaging. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 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.
src/mumble/Database.cpp
Outdated
| 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 |
|
Sorry for the long wait. But I have time to work on the PR again.
Oops. I don't remember which SQL-like language I've worked with that did have a similar feature, but SQLite isn't one of them. 😅 But I'm a bit at odds at how I should work this feature in. The first one is finding out a way to insert passwords anyway, even if the server isn't favorited. The more I think about it now, the less I like this idea. From the UI, it does make it clear when a password is stored. Either the checkbox is checked or it isn't. But I don't know if there are other ways to save a password that aren't behind a checkbox. Plus, then we're storing information about servers that aren't in the favorites already. Seems like we're currently not doing that. The other one is creating a clear rule when a user can and can't save a server. Then having the checkbox only be available if this condition is met. I feel like this can get convoluted. Do you have any ideas or feedback? |
…enabled This change was made so the future commit can optionally save passwords.
maxer137
force-pushed
the
save-password
branch
from
4164b3f to
39be854
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This change offers the functionality, but it's hardcoded within the UI FailedConnectionDialog option. So, if another part of the UI needs to check if they can save password, they'd have to re-implement the checks. 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
39be854 to
cf6bf51
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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