Improve error message when browser proxy cannot be found

[blessio]

A warning dialog-box pops up at startup stating that the proxy binary cannot be found.

Practically, this is done when updating native messaging manifests (that means checking happens not only on startup but also when advanced settings are changed). That is, if newly written settings results in no proxy found - then the warning dialog will popup also.
If the "updating of native messaging manifests" does not happen (Browser-Integration is off or a specific browser is not enabled etc.), then we assume that the user is advanced and intentionally sets up complex proxy strategy --> so checking is not done.

Screenshots

Testing strategy

• Tested the application on KDE Neon and run all unit-tests on Linux.
• did not test on Windows

Type of change

• ✅ Bug fix (non-breaking change that fixes an issue)
• ✅ improvent of Browser Integration Feature (change that adds functionality)

[droidmonkey]

I don't understand why this is necessary

[blessio]

I don't understand why this is necessary

From very practical point of view:

This simple and low-risk warning “fix” dramatically reduces the pain in those rare cases when the proxy is not placed correctly by the package manager or user (which is difficult to test and verify both for out-of-the box and manual configuration – cases, so such a mistake can easily happen but is hard for root-cause).
I may be biased but I lost some two weeks to figure out why my Linux installation did not work: trying keepassxc and Browsers in different package managers (snap, flatpack, etc … ), configurations and settings, and finally debugging to understand what was wrong … sincerely, it was painful…
Had I received an error message when the app starts, I would have known what to configure to make the app usable (without browser-integration keepassxc is not so easy to use).

From engineering and robustness point of view:

It is necessary because when the application is updating the native messaging manifests (as requested by the user) it is publishing (a set of) interfaces. If the interfaces published are known to be wrong, then they should not be... or at least the user should be informed that his/her request is not resulting in a working interface.

[droidmonkey]

trying keepassxc and Browsers in different package managers (snap, flatpack, etc … ), configurations and settings, and finally debugging to understand what was wrong

I don't understand this. Are you saying you installed keepassxc using 'official' sources and the proxy was not installed? That is simply not possible, especially for snap and flatpak.

Also a popup is not desired. I believe we already show an error message in the browser settings if you misconfigure the custom proxy setting.

[blessio]

trying keepassxc and Browsers in different package managers (snap, flatpack, etc … ), configurations and settings, and finally debugging to understand what was wrong

I don't understand this. Are you saying you installed keepassxc using 'official' sources and the proxy was not installed? That is simply not possible, especially for snap and flatpak.

I am pretty sure I installed it from an official place ... I started building the project simply because no matter what I did there was always an error message like "exchange of keys was not successful" in the browser extension.
It should be irrelevant, but FYI I am using the Linux distribution called "KDE Neon".

Looking at the pre-compiler complexity of the proxy executable path calculation (NativeMessageInstaller::getProxyPath()) - I am not surprised that it can sometimes go wrong).
Indeed, I found out that the only way to work around is to use the advanced setting of pointing manually to a custom proxy.

Also, a popup is not desired.

Here I agree completely :)
I wanted to show the warning in (A)a "statusbar" or (B) the yellow warning zone for snapshot builds (there is a yellow zone that says "WARNING: You are using an unstable build of KeePassXC.....").
For this to be done well (as opposed to a "patch") it would require function protype changes - thus, risking additional impact, which I wanted to avoid. Further, this required changes to more than one file (which although not so critical may also end up very painful to merge later).
If you think that the current approach of a popup is unacceptable then I would look into implementing (B).

Please let me know if this would be accepted.

I believe we already show an error message in the browser settings if you misconfigure the custom proxy setting.

I am sure that this is not the case, because I remember typing wrong string as custom proxy and it was accepted.
Besides, the error somehow happens not when one uses custom proxy but when using the "default" one. The proxy is most probably installed but the complex code of NativeMessageInstaller::getProxyPath() fails to locate it...

[droidmonkey]

You absolutely do not need to make function prototype changes to display a warning message. I wouldn't display it in the mainwindow warning box anyway. Just show a warning exactly how the custom proxy warning is done...

https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserSettingsWidget.cpp#L199-L201

[blessio]

Reworked the path validation by moving it completely to the ui.
Now the validation is much more robust. Tested manually on Linux and Windows.

1. Still possible to improve

1.1 UI

There is one more "gap": if (1) the user edits manually the custom-proxy setting and gets it wrong and (2) immediately his OK (or for some errors moves off the settings tab to another tab) then the error will be "shown" but not visible by the use because the page is not already there. Unfortunately, the only way to make sure that the user sees the error in such a case to show a pop-up (unless we want to show it in the top-most WarningWidget ..., which is even worse IMHO)

==> As this was never implemented - I consider it is not as a blocking point for the pull-request?

1.2 The data store layer

The settings-store layer (class BrowserSettings and the Proxy installer, that is) should be hardened to permit no wrong settings both when storing and even when settings are read.

2. Important notes

The old BrowserSettingsWidget::validateCustomProxyLocation() was reading the path from the data store (class BrowserSettings), instead of the ui and that is why it was accepting any value for customProxyLocation.

3. Screenshot(s)

4. REMARK:

The test "21 testmerge (Failed)": it is not because of the changes here because it was failing even before the changes in this pull-request.

Here is full log of the tests

Test project C:/Users/blessio/_prj/keepassxc-B/out/build/x64-Debug
      Start  1: testgroup
 1/41 Test  #1: testgroup ........................   Passed    0.05 sec
      Start  2: testkdbx2
 2/41 Test  #2: testkdbx2 ........................   Passed    0.06 sec
      Start  3: testkdbx3
 3/41 Test  #3: testkdbx3 ........................   Passed    1.39 sec
      Start  4: testkdbx4
 4/41 Test  #4: testkdbx4 ........................   Passed   37.19 sec
      Start  5: testkeys
 5/41 Test  #5: testkeys .........................   Passed    2.55 sec
      Start  6: testgroupmodel
 6/41 Test  #6: testgroupmodel ...................   Passed    0.05 sec
      Start  7: testentrymodel
 7/41 Test  #7: testentrymodel ...................   Passed    0.07 sec
      Start  8: testcryptohash
 8/41 Test  #8: testcryptohash ...................   Passed    0.04 sec
      Start  9: testsymmetriccipher
 9/41 Test  #9: testsymmetriccipher ..............   Passed    0.20 sec
      Start 10: testhashedblockstream
10/41 Test #10: testhashedblockstream ............   Passed    0.05 sec
      Start 11: testkeepass2randomstream
11/41 Test #11: testkeepass2randomstream .........   Passed    0.04 sec
      Start 12: testmodified
12/41 Test #12: testmodified .....................   Passed    9.34 sec
      Start 13: testdeletedobjects
13/41 Test #13: testdeletedobjects ...............   Passed    0.05 sec
      Start 14: testkeepass1reader
14/41 Test #14: testkeepass1reader ...............   Passed    0.14 sec
      Start 15: testopvaultreader
15/41 Test #15: testopvaultreader ................   Passed    1.29 sec
      Start 16: testupdatecheck
16/41 Test #16: testupdatecheck ..................   Passed    0.03 sec
      Start 17: testicondownloader
17/41 Test #17: testicondownloader ...............   Passed   29.84 sec
      Start 18: testautotype
18/41 Test #18: testautotype .....................   Passed   12.46 sec
      Start 19: testopensshkey
19/41 Test #19: testopensshkey ...................   Passed    3.77 sec
      Start 20: testentry
20/41 Test #20: testentry ........................   Passed    0.04 sec
      Start 21: testmerge
21/41 Test #21: testmerge ........................***Failed    0.30 sec
      Start 22: testpasswordgenerator
22/41 Test #22: testpasswordgenerator ............   Passed    0.04 sec
      Start 23: testpasswordhealth
23/41 Test #23: testpasswordhealth ...............   Passed    0.03 sec
      Start 24: testpassphrasegenerator
24/41 Test #24: testpassphrasegenerator ..........   Passed    0.06 sec
      Start 25: testhibp
25/41 Test #25: testhibp .........................   Passed    0.04 sec
      Start 26: testtotp
26/41 Test #26: testtotp .........................   Passed    0.04 sec
      Start 27: testbase32
27/41 Test #27: testbase32 .......................   Passed    0.02 sec
      Start 28: testcsvparser
28/41 Test #28: testcsvparser ....................   Passed    0.04 sec
      Start 29: testrandomgenerator
29/41 Test #29: testrandomgenerator ..............   Passed    0.02 sec
      Start 30: testentrysearcher
30/41 Test #30: testentrysearcher ................   Passed    0.04 sec
      Start 31: testcsvexporter
31/41 Test #31: testcsvexporter ..................   Passed    0.03 sec
      Start 32: testykchallengeresponsekey
32/41 Test #32: testykchallengeresponsekey .......   Passed    0.10 sec
      Start 33: testsharing
33/41 Test #33: testsharing ......................   Passed    9.25 sec
      Start 34: testdatabase
34/41 Test #34: testdatabase .....................   Passed    0.93 sec
      Start 35: testtools
35/41 Test #35: testtools ........................   Passed    0.03 sec
      Start 36: testconfig
36/41 Test #36: testconfig .......................   Passed    0.05 sec
      Start 37: testbrowser
37/41 Test #37: testbrowser ......................   Passed    0.11 sec
      Start 38: testcli
38/41 Test #38: testcli ..........................   Passed   40.53 sec
      Start 39: testgui
39/41 Test #39: testgui ..........................   Passed  119.39 sec
      Start 40: testguipixmaps
40/41 Test #40: testguipixmaps ...................   Passed    0.08 sec
      Start 41: testguibrowser
41/41 Test #41: testguibrowser ...................   Passed   25.55 sec
98% tests passed, 1 tests failed out of 41
Total Test time (real) = 295.40 sec
The following tests FAILED:
	 21 - testmerge (Failed)
Errors while running CTest
Output from these tests are in: C:/Users/blessio/_prj/keepassxc-B/out/build/x64-Debug/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

[blessio]

Would this Pull Request be accepted?

[droidmonkey]

Gotta review it but sure

[blessio]

Both amendments to cover review result are in commit b3478a5.

[blessio]

Names update done.
Review requested.

[varjolintu]

Names update done. Review requested.

Please use auto whenever possible. Also update the translations so CI won't complain about then.

[blessio]

Please use auto whenever possible.

It is possible in most or even all places with initializers in the declaration line. Still, I really prefer to keep the variable type static and predefined. That is, imho it is preferred that the compiler catches a potential issue when types change vs the convenience of less changes (and code checks when types change).

For instance, in the following code all these declarations could use auto but imho it is preferable to keep it explicit, because later in the code, not only method call of these types are used but often operator overrides (e.g. the bool inside an if(someBool) OR += for a QString).

    // check the rest of the dependency and set the warning message when needed for the complete dependency
    bool isValid = true;
    bool isUiBrowseIntegrationEnabled = m_ui->enableBrowserSupport->isChecked();
    QString warning = tr("<b>Error: </b>");
    QString info = tr("<b>Info: </b>");

_I believe that this should be acceptable. _
If it is not, please answer with a reference for precise guidelines so that I can comply.

---

Also update the translations so CI won't complain about them.

Done with commit 0a0f60b.

---

Further, all checks do pass now👍

I would highly appreciate if a more exhaustive list of issues is fed back at once instead of piece by piece.

[droidmonkey]

There is no such thing as dynamic typing in C++, auto is just a shortcut where there is no ambiguity on the type. Declaring a type absolutely does not prevent or protect you from implicit type conversions. For bool I prefer to write it out as there is no savings in programmer time, but for classes and templated types absolutely use auto where the compiler allows it.

I'll be doing a full review now and squashing to commits. Also please don't do the "merge latest develop" into a PR. It creates a lot of noise and a sloppy commit graph.

[blessio]

Thanks for the comment.

I agree with the "sloppy commit graph" comment ...
Imagined that squashing is standard practice for such a diverse and successful project.

Indeed, I would like to minimize the commits (or even just have one) and improve the commit graphs for future PRs. This would be hard or impossible if I cannot use the CI before I commit.
Is there any hint - how to do that?
The only way that comes to my mind is to make a new branch with the squashed commit but then it is not part of the running PR and would cause even further effort (not only for me but also for you.

Indeed, any hint how to use the project CI before committing (or with squashing) will be appreciated.

[droidmonkey]

You would just squash the commits, rebase onto develop, then run the tests locally on your computer. Adding commits linearly is fine, just rebase onto develop, don't merge it in. We will squash everything down to one commit before final merge.

I reviewed the PR and made wholesale changes to the code and messages. To be honest, I did not like your implementation. There was a ton of code and the messages were hard to understand. This is what the final implementation looks like (I also added red shading and tooltip to the custom proxy field when it is invalid):

When custom proxy is enabled:

When the installed proxy cannot be found: