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

FdoSecrets: Improve client executable path handling #6915

Merged
merged 7 commits into from
Oct 1, 2021

Conversation

Aetf
Copy link
Contributor

@Aetf Aetf commented Sep 14, 2021

After a busy summer, I'm back with some more time to contribute 😎

This PR improves the overall handling of FdoSecrets showing client executable paths to the user. It does the following

  • Check executable file existence as described in [RFC] fdosecrets: add optional confirmation to secret access #4733 (comment)
  • Show application PID and dbus address in the client list
  • When the executable file is inaccessible, depending on where the client name is shown
    • when shown inline, e.g. in notification text, where space is limited, clearly say that the path is invalid
    • when shown in auth dialog, show warning and print detailed info about the client
    • when shown in the client list, draw a warning icon

More about the auth dialog: someone in previous discussions (sorry I didn't remember exactly where) mentioned showing the process hierarchy. I liked the idea and it looks indeed cool. But I'm not sure if the function belongs to KPXC. Maybe it's better to just let the user use whatever tool already available (e.g. pstree) rather than duplicating the functionality.

Fixes #6459

Screenshots

Client list

Valid exe path
Screenshot_20210915_163242
Invalid exe path
Screenshot_20210915_164934

Notifications

bitmap

Auth dialog

When the path is valid, default to collapsed mode
image241

When the path is invalid, default to expanded mode with a warning
image345

Testing strategy

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ New feature (change that adds functionality)
  • ✅ Refactor (significant modification to existing code)

@Aetf Aetf force-pushed the fix/fdo-exe-path branch 3 times, most recently from 11c3463 to f01fba0 Compare September 15, 2021 22:03
@Aetf Aetf marked this pull request as ready for review September 15, 2021 22:58
@Aetf
Copy link
Contributor Author

Aetf commented Sep 15, 2021

The windows CI failure is unrelated. And I have no clue why the test fails when building AppImage on GCC in Release mode... probably some random error.

Other than that, this is ready :)

@droidmonkey
Copy link
Member

This is some seriously good stuff

@droidmonkey droidmonkey added this to the v2.7.0 milestone Sep 19, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

Merging #6915 (402db68) into develop (860fcfd) will decrease coverage by 0.10%.
The diff coverage is 42.65%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6915      +/-   ##
===========================================
- Coverage    63.78%   63.68%   -0.10%     
===========================================
  Files          330      330              
  Lines        41430    41576     +146     
===========================================
+ Hits         26423    26475      +52     
- Misses       15007    15101      +94     
Impacted Files Coverage Δ
src/fdosecrets/dbus/DBusMgr.cpp 51.90% <0.00%> (-7.05%) ⬇️
src/fdosecrets/dbus/DBusMgr.h 83.87% <ø> (ø)
src/fdosecrets/widgets/SettingsModels.cpp 44.27% <23.08%> (-7.02%) ⬇️
src/fdosecrets/dbus/DBusClient.cpp 64.95% <31.58%> (-9.41%) ⬇️
src/fdosecrets/widgets/AccessControlDialog.cpp 68.75% <82.09%> (+4.72%) ⬆️
src/fdosecrets/dbus/DBusClient.h 75.00% <83.33%> (+25.00%) ⬆️
src/fdosecrets/objects/Prompt.cpp 74.41% <100.00%> (+0.09%) ⬆️
...rc/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp 59.02% <100.00%> (+0.45%) ⬆️
src/core/Entry.cpp 83.89% <0.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 860fcfd...402db68. Read the comment docs.

@droidmonkey droidmonkey changed the title FdoSecrets: improve client executable path handling FdoSecrets: Improve client executable path handling Oct 1, 2021
@droidmonkey droidmonkey merged commit 60cfba8 into keepassxreboot:develop Oct 1, 2021
@Aetf Aetf deleted the fix/fdo-exe-path branch January 25, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FdoSecrets: warning when client executable path can't be determined
3 participants