8000 FEAT(client): Allow not saving passwords by maxer137 · Pull Request #6820 · mumble-voip/mumble · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

FEAT(client): Allow not saving passwords #6820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FEAT(client): Allow not saving passwords #6820

wants to merge 2 commits into from

Conversation

maxer137
Copy link
Contributor

When asked for a password, the user has the option to save or not save the password to the device.
Implements #1087

Checks

@Hartmnt Hartmnt added client feature-request This issue or PR deals with a new feature labels May 24, 2025
Hartmnt
Hartmnt requested changes May 26, 2025
View reviewed changes
src/mumble/PasswordDialog.ui Outdated
Comment on lines 50 to 59
<widget class="QCheckBox" name="qcbStorePassword">
<property name="toolTip">
<string>If unchecked, your password will not be stored on this device.</string>
</property>
<property name="layoutDirection">
<enum>Qt::LayoutDirection::LeftToRight</enum>
</property>
<property name="text">
<string>Save password on this device</string>
</property>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say let's call this option "remember" password instead of "save" or "store". I feel like the word "save" is a little overloaded with meaning. And I think "remember" is also the word used by other software for this use-case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about the options.

  • Remember me
  • Save password on this device
  • Keep me logged in on this device

I thought the storing/saving would make more sense in this scenario.

I didn't think "Remember me" would quite make sense, as there's not really a way to remove the password right now without getting an error that the password when connecting to a server.
Yes, other software uses this as well, but often when having log-in prompts. The mumble server isn't the one remembering the client. The mumble client is storing the password.

Similarly, for "keep me logged in on this device" wouldn't quite make sense because there is not really a way to "log out".

"Saving" the password made the 8000 most sense to me, as it most accurately describes what it does while still remaining user-friendly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this point is resolved already 😄

Do you not agree that this should be called Remember password?

If you are concerned because of the translation commit, you can just drop it using git and then re-create it using the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree that it should be called Remember password. I feel that “Store” or “Save” is more representative of the action which the button has. Where “Remember” is vague. “Remember” could refer to either the Server or the Client storing the password.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to favor save as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair enough 😆 But let's keep the wording of the checkbox consistent then qcbStorePassword -> qcbSavePassword

@maxer137 maxer137 force-pushed the save-password branch 2 times, most recently from ad1cac3 to 5b6d16b Compare May 26, 2025 16:29
maxer137
maxer137 commented Jun 1, 2025
View reviewed changes
Copy link
Contributor Author
@maxer137 maxer137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/mumble/PasswordDialog.ui Outdated
Comment on lines 50 to 59
<widget class="QCheckBox" name="qcbStorePassword">
<property name="toolTip">
<string>If unchecked, your password will not be stored on this device.</string>
</property>
<property name="layoutDirection">
<enum>Qt::LayoutDirection::LeftToRight</enum>
</property 8000 >
<property name="text">
<string>Save password on this device</string>
</property>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about the options.

  • Remember me
  • Save password on this device
  • Keep me logged in on this device

I thought the storing/saving would make more sense in this scenario.

I didn't think "Remember me" would quite make sense, as there's not really a way to remove the password right now without getting an error that the password when connecting to a server.
Yes, other software uses this as well, but often when having log-in prompts. The mumble server isn't the one remembering the client. The mumble client is storing the password.

Similarly, for "keep me logged in on this device" wouldn't quite make sense because there is not really a way to "log out".

"Saving" the password made the most sense to me, as it most accurately describes what it does while still remaining user-friendly.

src/mumble/PasswordDialog.ui Outdated
Comment on lines 50 to 59
<widget class="QCheckBox" name="qcbStorePassword">
<property name="toolTip">
<string>If unchecked, your password will not be stored on this device.</string>
</property>
<property name="layoutDirection">
<enum>Qt::LayoutDirection::LeftToRight</enum>
</property>
<property name="text">
<string>Save password on this device</string>
</property>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree that it should be called Remember password. I feel that “Store” or “Save” is more representative of the action which the button has. Where “Remember” is vague. “Remember” could refer to either the Server or the Client storing the password.

@Hartmnt
Copy link
Member
Hartmnt commented Jun 2, 2025

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.

maxer137 added 2 commits June 2, 2025 12:00
@maxer137
FEAT(client): Allow not saving passwords
a2f93b4 
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
TRANSLATION: Update translation files
bd36717
Hartmnt
Hartmnt reviewed Jun 2, 2025
View reviewed changes
Copy link
Member
@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I finally had time to compile and try this. While your change does what it advertises, I ran into two usability issues:

  1. 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.

  2. 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:

    mumble/src/mumble/Database.cpp

    Lines 721 to 730 in 41e381d

    void Database::setPassword(const QString &hostname, unsigned short port, const QString &uname, const QString &pw) {
    QSqlQuery query(db);
    query.prepare(
    QLatin1String("UPDATE `servers` SET `password` = ? WHERE `hostname` = ? AND `port` = ? AND `username` = ?"));
    query.addBindValue(pw);
    query.addBindValue(hostname);
    query.addBindValue(port);
    query.addBindValue(uname);
    execQueryAndLogFailure(query);
    }
    it says "UPDATE" and servers only contain favorites.
    Anyway, we should also hide or disable the checkbox in those cases.

@Hartmnt
Copy link
Member
Hartmnt commented Jun 2, 2025

2. it says "UPDATE" and servers only contain favorites.
Anyway, we should also hide or disable the checkbox in those cases.

Alternatively, we could UPDATE OR INSERT in the second case

@maxer137
Copy link
Contributor Author
maxer137 commented Jun 2, 2025

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.
I could also add this as a functionality of PasswordDialog.

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.

@Hartmnt
Copy link
Member
Hartmnt commented Jun 2, 2025

I'm wondering what is the best way to implement the password prompt.

I would suggest adding this as a possible option (or multiple options) for PasswordDialog, now that we have a custom class for that.

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.

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.

@maxer137
Copy link
Contributor Author
maxer137 commented Jun 2, 2025

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.

I would suggest adding this as a possible option (or multiple options) for PasswordDialog, now that we have a custom class for that.

Shall I get the fetching of the setting inside PasswordDialog or MainWindow?

@Hartmnt
Copy link
Member
Hartmnt commented Jun 2, 2025

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.

I would suggest adding this as a possible option (or multiple options) for PasswordDialog, now that we have a custom class for that.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hartmnt Hartmnt Hartmnt requested changes

@Krzmbrzl Krzmbrzl Krzmbrzl left review comments

Assignees
No one assigned
Labels
client feature-request This issue or PR deals with a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxer137 @Hartmnt @Krzmbrzl
0