Skip to content
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.

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

When reconnecting don't ask for login data again #1330

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johahauf
Copy link

No description provided.

@CendioOssman
Copy link
Member

I'm a bit cautious about this. Hanging on to passwords longer than necessary has historically been a source of security issues. An opt-in would be nice. Perhaps a check box on the dialog asking for the password?

@samhed samhed added the enhancement New feature or request label Sep 21, 2021
@johahauf
Copy link
Author

Yes, fully understand, I was also unsure about it, but just from personal use it is so convenient ;-)
Nice idea about the check box, would checking it by default be OK or should we save the last state in default.tigervnc/registry?

@CendioOssman
Copy link
Member

I think opt-in (i.e. unchecked) is the safer choice. I'm hoping we can live without the hassle of storing the choice. We don't really have any infrastructure for such things. The workflow should hopefully be <password> <tab> <space> <enter> to get the password remembered.

@johahauf
Copy link
Author

So like this?
image

Why not storing the last selection as part of the settings in the Windows registry or Linux default settings?

@CendioOssman
Copy link
Member

Yes, something like that. :)

The current settings storage system is based entirely around the configuration options. We currently don't have a way of storing anything other than configuration settings, and I'd like to avoid adding configuration options for "Don't ask me again" type of things as we can potentially have loads of those. So we need to build something new for those.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I've gone through it now and it's almost done. Just some minor tweaks left.

vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
UserDialog::UserDialog()
{
user_ = NULL;
password_ = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest more descriptive names on these, given that "user" and "password" occurs in multiple ways in this code.

vncviewer/UserDialog.cxx Show resolved Hide resolved
vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
vncviewer/UserDialog.cxx Outdated Show resolved Hide resolved
*user = strDup(username->value());
user_ = strDup(username->value());
Copy link
Member

Choose a reason for hiding this comment

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

This should also be if:ed with the value of the check box.

Copy link
Author

Choose a reason for hiding this comment

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

Why? any concerns about storing the user name in all cases?

Copy link
Member

Choose a reason for hiding this comment

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

Should hopefully not be as sensitive as the password, but might still be some privacy aspect that people prefer this not being remembered.

I guess it's not a must to change this, but I think it might be easier to understand that it's all or nothing what the viewer remembers for credentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants