LibSecret Map Lookup - #301
Conversation
|
That was specifically for the legacy KWallet API backend of QtKeychain. This was needed in plasma-nm!551 for the use case when the KWallet API backend remains active:
|
|
Hmm.. on closer look at the KWallet code:
So indeed, searching for only The |
| QVariantMap ReadPasswordJob::mapData() const | ||
| { | ||
| if (d->mode != JobPrivate::Map) | ||
| return QVariantMap(); | ||
|
|
||
| const QJsonDocument doc = QJsonDocument::fromJson(d->data); | ||
| if (!doc.isObject()) | ||
| return QVariantMap(); | ||
|
|
||
| return doc.object().toVariantMap(); | ||
| } |
There was a problem hiding this comment.
plasma-nm expects JSON text from type="map" entries when the KWallet backend is active:
https://invent.kde.org/plasma/plasma-nm/-/blob/master/kded/secretagent.cpp#L384
https://github.com/frankosterfeld/qtkeychain/blob/main/qtkeychain/keychain_unix.cpp#L450-L463
It should receive the same with the libsecret backend. And it should be returned from textData(). Please remove the mapData() interface.
Refer to #294 for the full changes on the KWallet backend.
| enum EntryType { Text = 0, Binary, Map }; | ||
|
|
||
| /** | ||
| * @return The type of data stored for this job's key(). | ||
| * @see Job::key() | ||
| */ | ||
| EntryType entryType() const; |
There was a problem hiding this comment.
Please remove. We should not introduce the backend-specific, legacy "map" concept to the generic public API. Further, the clients should already know what data type they're expecting from their entry.
(Note that the Map type that was added in #294 is hidden in the private implementation class.)
If a Binary-mode lookup would fall through, switch to Text mode and re-invoke secret_password_lookup_fn requesting type "map" so libsecret can return binary entries encoded as maps Address review: keep map lookup internal to libsecret Remove EntryType, entryType(), and mapData() from the public API. KWallet map entries found via libsecret are returned as JSON text through textData(), matching the KWallet backend behavior expected by plasma-nm. Co-authored-by: TradeCacheAI <TradeCacheAI@users.noreply.github.com>
|
I reverted the changes to what I had initially, to be as simple addition as possible. I had no idea that |
|
QtKeyChain has always only had When KWallet added Secret Service support, and was later refactored to use Secret Service as its primary backend, those legacy Map entries were converted to JSON, and incorrectly assigned New code will just use QtKeyChain's Text type interface with JSON values, which will be saved correctly as |
| arg->self->mode = QKeychain::JobPrivate::Text; | ||
| secret_password_lookup_fn(qtkeychainSchema(), nullptr, on_password_lookup, arg, | ||
| "user", arg->user.toUtf8().constData(), "server", | ||
| arg->server.toUtf8().constData(), "type", "map", | ||
| nullptr); |
There was a problem hiding this comment.
Please check that this correctly returns the map data as JSON via textData(). Since the data is coming from DBus, you might need to internally assign arg->self->mode = QKeychain::JobPrivate::Map, and then do some extra data juggling like in #294.
You can force the backend to KWallet using the QTKEYCHAIN_BACKEND environment variable, and compare the output to the libsecret backend with your changes. Both outputs should be identical.
|
Pinging @notmart to take a look at this also. |
2 participants
If a Binary-mode lookup would fall through, switch to Text mode and re-invoke secret_password_lookup_fn requesting type
mapsolibsecretcan return binary entries encoded as maps.I found this bug after switching to use Quickshell while continuing to use KWallet as my keychain.
NetworkManager had my passwords stored in KWallet as
mapsince I was using KDE6 previosly. Since the DE I'm using now doesn't advertise as any of the defined in the enum, the program was going through the default LibSecret case. While LibSecret can work with KWallet, it didn't had a case to look formaptype of passwords in the keyring, onlytextandbase64.I saw that with the latest commit,
mapsupport was introduced, so hopefully this will be helpful addition.Also I'm currently using those changes, so it'll be convenient to not have to patch each new version.