Configure Freedesktop Secret Service Aliases #12252
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cynhr
force-pushed
the
feature/fdosecrets-alias
branch
from
807574f to
8638e57
Compare
droidmonkey
added
feature: Secret Service
pr: new feature
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for configuring Freedesktop Secret Service collection aliases by UUID, including storage, lookup, UI, and application in the service layer.
- Track open databases by their public UUID for alias lookup
- Introduce a new QVariantMap config field and persist alias⇄UUID mappings
- Provide a settings UI and apply configured aliases in the FdoSecrets service
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gui/DatabaseTabWidget.{h,cpp} | Map tabs by public UUID and expose lookup/update methods |
| src/fdosecrets/widgets/SettingsWidgetFdoSecrets.{ui,cpp} | Add “Collection Aliases” settings view and delegate |
| src/fdosecrets/widgets/SettingsModels.{h,cpp} | Implement SettingsAliasesModel for alias⇄UUID mapping |
| src/fdosecrets/objects/Service.{h,cpp} | Apply configured aliases and handle dynamic aliasing |
| src/fdosecrets/FdoSecretsSettings.{h,cpp} | Persist and signal collection alias changes |
| src/core/Config.{h,cpp} | Register new config key for alias mappings |
| share/translations/keepassxc_en.ts | Add translations for new UI strings |
Comments suppressed due to low confidence (1)
src/fdosecrets/widgets/SettingsModels.cpp:250
- There are no unit tests covering SettingsAliasesModel. Consider adding QTest-based tests for rowCount, data(), setData() and moveAlias() scenarios.
SettingsAliasesModel::SettingsAliasesModel(const DatabaseTabWidget* dbTabs, QObject* parent)
droidmonkey
reviewed
Jul 2, 2025
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cynhr
pushed a commit
to cynhr/keepassxc
that referenced
this pull request
Jul 9, 2025
Remove the collectionAliasesChanged() signal from FdoSecretsSettings to avoid making it a QObject. Instead connect to Config::changed and check which Config key changed. This does break the abstraction layer that FdoSecretsSettings was between Service and Config, and requires comparing the key each time any config value is updated, but averts the QObject overhead for FdoSecretsSettings. Suggested-by: Jonathan White <[email protected]> (keepassxreboot#12252 (comment))
cynhr
force-pushed
the
feature/fdosecrets-alias
branch
2 times, most recently
from
5c1f58f to
b16c024
Compare
|
So far, there are only tests for checking, that configured aliases are applied (i.e. exposed to DBus). I'd like to also add test for editing a I did have some problems running the tests locally: |
cynhr
force-pushed
the
feature/fdosecrets-alias
branch
3 times, most recently
from
7a504c2 to
44b0a41
Compare
added 8 commits
August 21, 2025 16:58
Add a map from public UUID to database tab to DatabaseTabWidget, to allow efficiently accessing Databases by public UUID. Similar to Database::s_uuidMap. This commit does not add any user-facing features. While independently created UUIDs usually don't collide, the public UUID could be manipulated, or simply duplicated by copying the file. This will simply ignore all databases opened while another database with the same public UUID is already open (even if that first database is later closed, until the second database is re-opened).
Add a setting to store the Freedesktop Secret Service aliases. Setting e.g. the "default" alias indicates, to which database most applications should store new secrets. This is a QVariantMap (aka QMap<QString, QVariant>) as opposed to a QMap<QString, QUuid>, as only the former can be stored to QSettings. Converting a QVariantMap to QMap<QString, QUuid> would require copying the entire map, calling QVariant::toUuid on every value. This commit does not yet add the user interface for configuring this setting, nor does it apply the configured aliases to the actual D-Bus service.
Create and remove Freedesktop Secret Service aliases as configured in FdoSecretSettings, and store aliases created/removed via D-Bus to FdoSecretSettings. If the "default" alias has not been configured, this works exactly like it did before. If the "default" alias has been configured, but the corresponding database isn't currently opened (not even opened-but-locked), no "default" alias is exposed. This prevents secrets from accidentally being written to another database (imagine a database being routinely shared with other people, which now suddenly contains an access token for your private email, just because the file system containing your private passwords wasn't mounted). Providing no "default" alias is already what we're doing if no database tab is opened. The implementation would have preferred a "database → Set<alias>" mapping (compare `Service::applyCollectionAliasSettings`), but "alias → database" is the natural model making invalid states (multiple databases owning the same alias) unrepresentable, requiring less validation code.
Fixes keepassxreboot#8479 The QComboBox'es created by DatabaseUuidDelegate::createEditor are not updated when database tabs are created or destroyed. A new editor seems to be created each time the user starts editing a field, so this should not be a problem.
Remove the collectionAliasesChanged() signal from FdoSecretsSettings to avoid making it a QObject. Instead connect to Config::changed and check which Config key changed. This does break the abstraction layer that FdoSecretsSettings was between Service and Config, and requires comparing the key each time any config value is updated, but averts the QObject overhead for FdoSecretsSettings. Suggested-by: Jonathan White <[email protected]> (keepassxreboot#12252 (comment))
This operator+ is deprecated since Qt6.2: https://doc.qt.io/archives/qt-6.2/qmap-iterator.html. QMap is implemented as a red/black tree, presumably without storing subtree sizes, so this is a linear lookup. We need to call this for each displayed alias (multiple times), so linear complexity for lookup by rank is a bummer. But presumably nobody's going to have that many aliases, that this actually matters. Suggested-by: Copilot
Formulate a single case switch-statement as an if-statement. If we ever want to add additional cases, just turn it back into a switch-statement. Avoid calling QComboBox::setItemData with out-of-bounds indices. While QComboBox::insertItem documents that it works with larger indices, QComboBox::setItemData does not. So instead use the actual real index for both. (Note that QComboBox::addItem also just calls "insertItem(count(), ...)", so QComboBox::addItem wouldn't be any more efficient, but this way we see both calls actually using the same index.) Suggested by CodeQL (switch -> if) and Copilot (QComboBox indices).
Verify that configured Freedesktop Secret Service aliases are exposed on the DBus interface. Also run some tests against SettingsAliasesModel, using Qt's QAbstractItemModelTester, which tries to find violations of QAbstractItemModel's invariants. Including that we apparently shouldn't return flags for invalid indices, so check the index's validity in SettingsAliasesModel::flags().
cynhr
force-pushed
the
feature/fdosecrets-alias
branch
from
44b0a41 to
a5531a0
Compare
|
This should be ready for review now. The MacOS failure (at a glance) looks unrelated (something in |
|
I need to reset macos, don't worry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow configuring the exposed Freedesktop Secret Service aliases. This is especially relevant for setting the
defaultalias.Fixes #8479.
This is split into 4 commits:
Service/CollectionsSome notes:
I'm not storing the databases' file paths (which was suggested in one of the issues). I'd be a bit surprised to have a Secret Service D-Bus request create a new database tab (as opposed to only offering to unlock existing database tabs)? But maybe that's just me … If someone ends up implementing automatic database loading, those file paths should probably go into local (not roaming) storage (maybe as an UUID → path mapping?), as file paths are not necessarily identical on all systems.
FdoSecrets::Service::setAlias, which is exposed to D-Bus, I believe), which is a change from the current behaviour. As a user, I find it a bit surprising that new collection can be created / aliased via D-Bus, but that seems to be the spec.QComboBox?// static constexpr still requires definition before c++17forSettingsAliasesModel::ColumnNamesfor consistency with the otherColumnNames, even though the code uses C++17 features (std::as_const) and the top-levelCMakeLists.txtmandates C++17 by now.I did not use any generative AI myself. I did apply suggestions created by Copilot etc., posted to this PR.
Screenshots
Testing strategy
None so far.
Type of change