FIX: Password generator skips special characters

[ba32107]

Type of change

• ✅ Bug fix (non-breaking change which fixes an issue)

Description and Context

Fixes #3064
Fixes #2749
This issue was introduced by #1841. I found three problems with the code:

• Even though the generator/SpecialChars setting was saved properly, it was never loaded.
• The generator/AdvancedMode setting was never saved.
• The code that initializes the password generation options uses the isVisible method of a UI component to decide which initial character sets to include. When this code first runs (on application startup), the UI is not ready yet at all, therefore the correct code path never runs. Any subsequent action on the UI invokes this code again (this time the visibility check runs correctly), which fixes all settings, as reported in both issues.

I added both settings to the appropriate load/save functions. I also moved the password generator initialization logic to the proper event handler from the constructor.

Testing strategy

• Tested manually against the bugs reported in Password generator skips special characters #3064 and Update Github templates #2479.
• Checked config file to verify that the generator/SpecialChars and generator\AdvancedMode setting is saved and loaded correctly.

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

[droidmonkey]

Sweet you beat me to it!

[droidmonkey]

We need to evaluate the special character groupings that are currently available. Right now I rarely am able to pick a grouping that satisfies stringent site requirements. Usually there are "safe" special characters like !@#$%& that almost all sites accept. That should be part of the basic buttons. The advanced buttons can add to that group allowing punctuation, extended ascii, extended symbols, etc.

We also have a request to add an include free-form text edit that would manually add to any buttons selected. That might be overkill if the buttons are better aligned with what "most people" would use.

[ba32107]

I almost never use the advanced options, I usually just pick the special characters on the simple panel - works well on most sites I use.

Not sure I understand the second improvement you mentioned. Is there an issue for it?

[droidmonkey]

See #3271 and #2313

[ba32107]

The new commit I made only resolved the conflicts in the CHANGELOG file (I squashed all changes into one commit, but didn't change anything now apart from the CHANGELOG), so the Windows build failing should be independent. I think there might have been changes in the develop branch that cause the test to fail. Is it perhaps this commit? I'm only assuming because TOTP-related changes are in it:
3022ca9

I am on Ubuntu, all tests pass for me.

EDIT: all green

[phoerious]

I wonder if "Pick characters from every group" should default to off and we replace it with another more visible but temporary switch in case you are trying to generate a password for a website with stupid guidelines. I don't like non-random processes interfering with strong pseudo-random password generation by default.

[ba32107]

Just to confirm that I understand you correctly: do you mean that if the "Pick characters from every group" is enabled by default, that adds a certain predictability to the password? If so, yes I agree. And I presume the new switch you're proposing would be called exactly the same, but would be disabled by default.

I like this suggestion. Not sure if it should be done as part of this PR though - it's a pretty nasty bug that has bit many people. Would be great to roll it out as soon as possible.

[phoerious]

It does make the random password less random, yes. Still quite secure, but less random than it could be.

[droidmonkey]

Most sites with stupid password rules require at least 1 (sometimes 2) characters from each group. Security is impacted by the site itself, not us.

[droidmonkey]

@ba32107 is this ready or are there more corrections you would like to add?

[ba32107]

Sorry, I didn't realize you were waiting for me to make further changes. I considered this PR as ready, because it addresses an important bug. I assumed that we'll tackle @phoerious's suggestion in a separate issue.