Yet another example of bad naming. I suppose this should be renamed to attributes
.
And if the Secret Service API is called, then one of the Secret Service classes'
calledFromDBus()
should returntrue
, whileKWalletD::calledFromDBus()
and the rest should returnfalse
. Is that correct?
Yes, it is.
Since the KWalletAPI itself has only one QDBusContext
's derived class, it is simplier to use it's calledFromDBus()
(instead of calledFromDBus()
in any of Secret Service related objects).
Also, since the Secret Service API widely uses properties, we cannot rely on calledFromDBus()
in Secret Service classes, because it always returns false inside the property handler (maybe this is a bug).
The is done by calling the itemAttributes().updateLastModified()
in KWalletFreedesktopCollection::onItemChanged()
.
It works as intended. It is implemeted via member-function QDBusContext::calledFromDBus()
from KWalletD's base class (and because this is protected function, we need to reimplent it in the derived class).
QDBusContext::calledFromDBus() returns true if the internal pimpl pointer is not nullptr. This can only happen when handling the QDBusConnectionPrivate::deliverCall()
in qdbusintegrator.cpp.
The qt doc does not tell us, is this function works within QDBusContext's context or not: https://doc.qt.io/qt-6/qdbuscontext.html#calledFromDBus
But this is a member function, and the class is named QDBusContext, so the function should respect the context calledFromKWalletAPI()
without this function.
The scoped enum (enum class) cannot be used, because QtDBus adaptor cannot generate code with cast from enum class to its underlying type.
Also the most of return codes are used in if conditions which is not allowed for enum classes without explicit static_cast<bool>(...)
.
In the end, I cannot figure out the logic in existing return codes. In case of removeEntry()
we will get -1 if wallet not found and -3 if the Backend::removeEntry()
failed. But in case of failed renameEntry()
we will get -1 for all error cases.
So, I think there are two real cases:
Therefore I don't see much point to use enums/named constants here. To introduce them first of all we should introduce good error codes/constants for every case (which I would rather not do, because it will affects the KWalletAPI).
Is there an idea how to name them?
Maybe something like that:
enum KWalletDRc {
RcSuccess = 0,
RcWalletNotFound = -1,
RcRemoveEntryFailed = -3
}
?
Also 0 returns when folder not exists. Seems to be a mistake.
The existing test is a mess
It definitely should be rewritten, and I hope to do this soon. This is should be done before any serious refactoring (and also implementing the https://bugs.kde.org/show_bug.cgi?id=458644). (or with refactoring because I doubt that the current code will allow the test to cover all points)
New change iteration with proper bugfix and some renaming flow refactoring has been pushed. Please check it out.
The Secret Service's internal data changes have been moved to KWalletFreedesktopItem::setEntryLocation()
(KWalletFreedesktopItem::uniqueLabel()
in the past). The KWalletFreedesktopService::entryRenamed()
now only calls the setEntryLocation()
for the corresponding item. In the case where folder is changing, the setEntryLocation()
is called manually from setLabel()
(not the entryRenamed()
, because it requires converting names and contains the search which is useless in this case)
So emitting entryRenamed()
after failed renaming is the real reason for this bug (the current fix is no good, I will redo it).
However, we face a some sort of cross-API-call problem when renaming is started through Secret Service API:
KWalletFreedesktopItem::setLabel()
change the item label and callsKWalletD::renameEntry()
.KWalletD::renameEntry()
performs renaming, schedule a sync, and emitsentryRenamed()
signal.- Subscribed member function
KWalletFreedesktopService::entryRenamed()
trying to update the item label >second time, but fails, because the label was already renamed (search for old name fails). So the function >returns prematurely without doing anything.KWalletFreedesktopItem::setLabel()
continue its execution and update other SecretService API related data (attributes).
As I understand, the problem is that the Secret Service API related code is duplicated: in this particular case the KWalletFreedesktopItem::setLabel()
and KWalletFreedesktopService::entryRenamed()
have the same code that changes SecretService label/attributes and each of them is executed in different circumstances depending on which API started the rename. Please correct me if I am wrong
If the call comes from KWallet API it works almost same as you described in this particular case (or maybe I missed something).
The 3 and 4 steps are:
KWalletD::renameEntry()
performs renaming through KWallet::Backend
, schedules a sync, and emits entryRenamed()
signal even the renaming was unsuccessful(!!!) (and this is my fault: it seems like it should be emitted only if rc == 0
).KWalletFreedesktopService::entryRenamed()
updates the Secret Service API related data only (so it never propagates back to KWalletD
and never calls any KWalletD
function).However, we face a some sort of cross-API-call problem when renaming is started through Secret Service API:
KWalletFreedesktopItem::setLabel()
change the item label and calls KWalletD::renameEntry()
.KWalletD::renameEntry()
performs renaming, schedule a sync, and emits entryRenamed()
signal.KWalletFreedesktopService::entryRenamed()
trying to update the item label second time, but fails, because the label was already renamed (search for old name fails). So the function returns prematurely without doing anything.KWalletFreedesktopItem::setLabel()
continue its execution and update other SecretService API related data (attributes).This is a really complex and unexpected call sequence. Maybe this can be fixed by introducing some mechanism to determine from which API the execution is started - this is the exactly what we want to know in the third point from the list above. Searching by the old name is just an ad hoc solution to determine this.
(Sorry for my absence, I'll try to answer you in other threads as soon as I can).
Slava Aseev (6e107f72) at 03 Oct 11:34
Sercret Service API: cancel entry renaming if new name is already i...
Slava Aseev (32e5bef0) at 03 Oct 11:30
Sercret Service API: cancel renaming if new name is already in use
... and 149 more commits
Slava Aseev (b24a3a76) at 03 Aug 19:47
Yes, it seems to be the same issue.
I think the solution is to mutex inside the helper.
How does this help? I can't figure out.
The problem is that all isuserknown
executions scheduled at the same time (with different username argument) returns the same result as the first isuserknown
execution.
There is a minimal reproducible example for this behaviour: https://invent.kde.org/ptrnine/kauth-action-bug
The same happens with isuserknown
action: all isuserknown
executions returns the result for the first user here: https://invent.kde.org/network/kdenetwork-filesharing/-/blob/master/samba/filepropertiesplugin/usermanager.cpp#L101
This patch can be used to test this behaviour in the filesharing plugin:
diff --git a/samba/filepropertiesplugin/usermanager.cpp b/samba/filepropertiesplugin/usermanager.cpp
index 7ae0f22..66bd2e7 100644
--- a/samba/filepropertiesplugin/usermanager.cpp
+++ b/samba/filepropertiesplugin/usermanager.cpp
@@ -6,6 +6,7 @@
#include "usermanager.h"
#include <KUser>
+#include <QDebug>
#include <QFile>
#include <QRegularExpression>
#include <QProcess>
@@ -99,6 +100,7 @@ void User::resolve()
connect(job, &KAuth::ExecuteJob::result, this, [this, job] {
job->deleteLater();
m_inSamba = job->data().value(QStringLiteral("exists"), false).toBool();
+ qWarning() << "user" << m_name << "exists:" << m_inSamba;
Q_EMIT inSambaChanged();
Q_EMIT resolved();
});
@@ -156,6 +158,7 @@ void UserManager::load()
const QString currentUserName = KUser().loginName();
const QStringList nameList = getUsersList();
for (const auto &name : nameList) {
+ qWarning() << "load(): processing user" << name;
++m_waitingForResolution;
auto user = new User(name, this);
connect(user, &User::resolved, this, [this] {
For example, we have two users:
user test
:
# pdbedit --debuglevel=0 --user test
test:500:
and user another-user
:
# pdbedit --debuglevel=0 --user another-user
Username not found!
so the output will be:
load(): processing user "Everyone"
load(): processing user "test"
load(): processing user "another-user"
user "test" exists: true
user "another-user" exists: true
but should be false
for the another-user
.
I don't know for sure if something is being done wrong here, or if it's a flaw/bug (feature!?) in the KAuth.
Slava Aseev (a8b826ec) at 21 Jul 15:03
Add sources