FDO Secrets GUI separation

[Neverous]

I'm trying to run KeePassXC headless with CLI mode. But I also like to keep using Freedesktop.org Secret Service integration as my keyring provider for various other applications on the remote to access secrets I keep in the database. Unfortunately current fdosecrets implementation is heavily dependent on GUI and is not supported in CLI mode.

This PR is a first part of trying to make fdosecrets work with CLI. It only focuses on separating fdosecrets internals from GUI, which I think is worth even of itself? To that end I moved all the methods that directly depended on GUI to the top-level FdoSecretsPluginGUI implementation using virtual methods (allowing to later make FdoSecretsPluginCLI that will replace those methods with CLI supporting versions). Since the plugin relied heavily on GUI objects (DatabaseWidget and DatabaseTabWidget) it turned into quite a big refactoring while trying to move them out :cold_sweat:, we'll see how it goes.

Other notable things:

• GUI dependent functions were changed to be blocking to avoid chaining sockets/signals/adding loop.exec() in random places just to fetch the actually needed values out of UI responses (essentially moved loop.exec()s into them wherever necessary), this also simplified D-Bus Prompts making them all synchronous as well.
• Because they depended on GUI logic patterns, some Prompts tests needed adjustments. But similar logic was already present in other cases so this just normalized their handling across the fdosecrets GUI tests.
• In src/Gui/group/GroupModel.cpp I had to add the links to the group model object in QObject::connect as otherwise the signals links could outlive the objects and throw errors (caught randomly during testing). And similar in some other QObject::connects.
• And I couldn't help myself and normalized D-Bus name everywhere I found it :sweat_smile:

Screenshots

N/A

Testing strategy

1. I run all the tests for the test suite, they seem to work (though some needed updates because of change in prompts implementation)
2. Running with these edits for last few months without any issues (though my use cases probably aren't any near exhaustive)

Type of change

• ✅ Refactor (significant modification to existing code)

[codecov-commenter]

Codecov Report

Base: 64.44% // Head: 64.34% // Decreases project coverage by -0.11% :warning:

Coverage data is based on head (6e69916) compared to base (56307e6). Patch coverage: 82.40% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #8591      +/-   ##
===========================================
- Coverage    64.44%   64.34%   -0.11%     
===========================================
  Files          340      345       +5     
  Lines        44142    44053      -89     
===========================================
- Hits         28446    28342     -104     
- Misses       15696    15711      +15     

Impacted Files	Coverage Δ
src/fdosecrets/FdoSecretsSettings.cpp	100.00% <ø> (ø)
src/fdosecrets/FdoSecretsSettings.h	100.00% <ø> (ø)
src/fdosecrets/dbus/DBusClient.h	75.00% <ø> (ø)
src/fdosecrets/dbus/DBusMgr.h	83.87% <ø> (ø)
src/fdosecrets/dbus/DBusObject.h	84.62% <ø> (-15.38%)	:arrow_down:
src/fdosecrets/dbus/DBusTypes.h	100.00% <ø> (ø)
src/fdosecrets/objects/Prompt.h	66.67% <ø> (-14.58%)	:arrow_down:
src/fdosecrets/objects/Service.h	100.00% <ø> (ø)
src/fdosecrets/widgets/SettingsModels.cpp	44.27% <0.00%> (ø)
src/fdosecrets/FdoSecretsSettingsPage.cpp	20.83% <20.83%> (ø)
... and 26 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[droidmonkey]

@louib we should merge this in after your gui split PR. @Aetf do you have a reasonable set of your own tests you run to make sure everything works the way you expect?

@Neverous great job. This actually plays into our ongoing refactor to segregate the gui from the core operations.

[Aetf]

This is great! I've been wanting to separate the logic from GUI ever since I first touched the codebase, but it was not possible as FdoSecrets needs an object representing the database consistently whether it's locked or not. And DatabaseWidget is the best I could have at the time.

@Neverous I just look at your description and not the code yet, so the following might not be applicable. I'm just dumping what I have in mind originally, in case it's helpful:

I think I already tried to separate bits related to GUI fairly well, some notable points

• anything that needs to talk to other parts of the GUI is ultimately a method on the Service object
• DatabaseTabWidget is only used to get new database signals and can be replaced with something else that can emit similar signals
• DatabaseWidget is the backend for Collection, because it tracks the lock/unlock state. You need to come up with something else that can track the lock/unlock for the same db.
• Prompts are interesting because ultimately you will need to show dialogs and alike, so even the CLI version has to link to GUI libraries

Regarding tests, unit tests should cover all the things I care about. The tests are implemented in a way that also covers the D-Bus code path (It will actually go through all the D-Bus message mangle/demangle). For extra certainty, you could try running tests from external projects, like python-secretstorage, against KPXC.

[michaelk83]

Prompts are interesting because ultimately you will need to show dialogs and alike, so even the CLI version has to link to GUI libraries

FdoSecretsPluginCLI can assume that all prompts are disabled, or display simplified prompts via TTY, if one is available. It doesn't need the GUI.

[Aetf]

display simplified prompts via TTY

I would assume running fdosecrets on cli means running the program as a service, in which case there's no TTY.

It can also call another binary to display the dialog and get responses, like kdialog, or some simple helper we write.

[michaelk83]

I would assume running fdosecrets on cli means running the program as a service, in which case there's no TTY.

Usually true. There's no guarantee of a desktop environment, either.

If there isn't a desktop, there's at least the currently active CLI session's TTY. But then again, I don't know a lot of CLI programs that would need Secret Service. Mostly python scripts.

It can also call another binary to display the dialog and get responses, like kdialog, or some simple helper we write.

For simple text prompts like the master password, there's the whole pinentry mechanism (part of GPG), which can be configured to either TTY or GUI as desired. The problem with it is that it usually tries to read the string from Secret Service first, which in our case is undesirable. There's an esoteric setting in gpg-agent to disable that, but even advanced users are unlikely to be familiar with it. So that would be a common pitfall.

Then there's the possibility of key files and YubiKeys, or multiple databases, so a simple text prompt may not be enough. So really, if OP wants to run this headless, there should be some automated way to select the DB and provide credentials, then treat all other prompts as disabled after that. Maybe providing credentials only at startup is enough.

This can be configurable, since even among those who may not want to run the main GUI, not everyone would be running this headless and without a desktop. But probably start with the simpler option first.

[Neverous]

anything that needs to talk to other parts of the GUI is ultimately a method on the Service object

Almost, there was also for example GuiTools::confirmDeleteEntries/deleteEntriesResolveReferences used in Collection code directly and one of the prompts is creating AccessControlDialog when it needs it :wink: but generally true.

As for DatabaseTabWidget, DatabaseWidget and tracking lock/unlock state of the Database: I changed the Collection to use the "database name" (file path or temporarily name from the metadata that was conveniently available already) as the handle (instead of the DatabaseWidget) and moved all the lock/unlock tracking externally. Now the Collection just expects databaseUnlocked/databaseLocked calls that update it with current Database object whenever necessary from the outside - and I moved the logic of tracking DatabaseWidget signals that was previously inside Collection to FdoSecretsPluginGUI (alongside the direct GUI calls/widgets mentioned above).

After moving the GUI stuff all into one place, the "user actions" that are possible from the FdoSecrets integration seems to be:

• request specific database lock
• request specific database unlock
• request any database unlock
• create new database
• request entries removal
• request entries unlock/access

And for anyone curious my current setup for CLI (not very beautiful but will probably push PR of it in the future, if only for reference, but probably after this is cleaned up :stuck_out_tongue: ), kind of as michaelk83 predicted above, is pretty basic: as I am connecting to the remote instance I run CLI (keepasxc-cli open ...) in tmux/screen window so I simply added the necessary support in the "interactive mode" locking/unlocking the singular database with fdosecrets there (and ssh-agent actually as well) and simply don't support database user actions (lock/unlock/create). And for the other confirmation requests I extended the LineReaders to show them in the keepassxc-cli terminal and wait for user responses there instead of CLI commands.

[Aetf]

Hi @Neverous, any updates on this? I plan to work on some new features that will touch both the UI and the logic part. Since this PR changes the code structure significantly, I want to check if I should base my work on this branch to avoid merge conflicts if this PR is more or less in a ready state. Otherwise, I'll still need to base on the latest develop branch, and there will be more conflicts.

[Neverous]

Well I'm waiting for a review :thinking:? and maybe https://github.com/keepassxreboot/keepassxc/pull/8480 to rebase on top of it before that (https://github.com/keepassxreboot/keepassxc/pull/8591#issuecomment-1280082953)