From 7a761a362ccb8a1d91c0d82cd6472b57ddaf0948 Mon Sep 17 00:00:00 2001 From: Stephen Lyons Date: Sat, 20 Nov 2021 22:14:58 +0000 Subject: [PATCH] Improve: do not try load-/sav-ing password securely when storing them portably I was annoyed by all the error messages - both on command line from Mudlet itself and also from (on Linux/FreeBSD) the "wallet" system about not finding passwords from the secure storage system when I have explicitly said that Mudlet is to use the (insecure) PORTABLE method. It is worth noting that the second type of warnings (from the DE/OS) can be highly noticably as they are popups that require clicking to dismiss them! I also took the liberty of tweaking related command line debugging texts to be formatted slightly differently so that they matched the format I now tend to use: "class::method name(...) WORD - text." with terminal punctation; insertion of double quotes around strings; and spaces as required manually rather than letting the Qt QDebug class doing it on every element of a message. Signed-off-by: Stephen Lyons --- src/Host.cpp | 16 ++++++++++++++-- src/dlgConnectionProfiles.cpp | 32 ++++++++++++++++++++++---------- src/mudlet.cpp | 5 +++-- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/Host.cpp b/src/Host.cpp index b7a834c3aa5..3f04a9414fe 100644 --- a/src/Host.cpp +++ b/src/Host.cpp @@ -459,7 +459,16 @@ Host::Host(int port, const QString& hostname, const QString& login, const QStrin mDiscordDisableServerSide = optin.toInt() == Qt::Unchecked ? true : false; } - loadSecuredPassword(); + if (mudlet::self()->storingPasswordsSecurely()) { + // This will not insert the password into the mPass member immediately: + loadSecuredPassword(); + } else { + QString password{readProfileData(QStringLiteral("password"))}; + if (!password.isEmpty()) { + setPass(password); + password.clear(); + } + } if (mudlet::scmIsPublicTestVersion) { thankForUsingPTB(); @@ -2692,6 +2701,7 @@ void Host::setDisplayFontFixedPitch(bool enable) mDisplayFont.setFixedPitch(enable); } +// Only used in the constructor - and only when passwords are stored securely void Host::loadSecuredPassword() { auto *job = new QKeychain::ReadPasswordJob(QStringLiteral("Mudlet profile")); @@ -2703,9 +2713,11 @@ void Host::loadSecuredPassword() connect(job, &QKeychain::ReadPasswordJob::finished, this, [=](QKeychain::Job* job) { if (job->error()) { const auto error = job->errorString(); + // Report to debug output the problem if it was something other than the password not being found if (error != QStringLiteral("Entry not found") && error != QStringLiteral("No match")) { - qDebug() << "Host::loadSecuredPassword ERROR: couldn't retrieve secure password for" << getName() << ", error is:" << error; + qDebug().nospace().noquote() << "Host::loadSecuredPassword() ERROR - could not retrieve secure password for \"" << getName() << "\", error is: " << error << "."; } + } else { auto readJob = static_cast(job); setPass(readJob->textData()); diff --git a/src/dlgConnectionProfiles.cpp b/src/dlgConnectionProfiles.cpp index 33c2981f9f7..ef6a4274448 100644 --- a/src/dlgConnectionProfiles.cpp +++ b/src/dlgConnectionProfiles.cpp @@ -338,6 +338,7 @@ void dlgConnectionProfiles::writeSecurePassword(const QString& profile, const QS job->start(); } +// Only used by migrateSecuredPassword(...) void dlgConnectionProfiles::deleteSecurePassword(const QString& profile) const { auto* job = new QKeychain::DeletePasswordJob(QStringLiteral("Mudlet profile")); @@ -490,7 +491,9 @@ void dlgConnectionProfiles::slot_save_name() return; } - migrateSecuredPassword(currentProfileEditName, newProfileName); + if (mudlet::self()->storingPasswordsSecurely()) { + migrateSecuredPassword(currentProfileEditName, newProfileName); + } setItemName(pItem, newProfileName); @@ -908,13 +911,18 @@ void dlgConnectionProfiles::slot_item_clicked(QListWidgetItem* pItem) // by the copy method if (!mCopyingProfile) { character_password_entry->setText(QString()); - loadSecuredPassword(profile_name, [this, profile_name](const QString& password) { - if (!password.isEmpty()) { - character_password_entry->setText(password); - } else { - character_password_entry->setText(readProfileData(profile_name, QStringLiteral("password"))); - } - }); + if (mudlet::self()->storingPasswordsSecurely()) { + loadSecuredPassword(profile_name, [this, profile_name](const QString& password) { + if (!password.isEmpty()) { + character_password_entry->setText(password); + } else { + character_password_entry->setText(readProfileData(profile_name, QStringLiteral("password"))); + } + }); + + } else { + character_password_entry->setText(readProfileData(profile_name, QStringLiteral("password"))); + } } val = readProfileData(profile_name, QStringLiteral("login")); @@ -1229,7 +1237,9 @@ void dlgConnectionProfiles::setCustomIcon(const QString& profileName, QListWidge profile->setIcon(icon); } -// When a profile is renamed, migrate password storage to the new profile +// When a profile is renamed, migrate password storage to the new profile. +// This is only used by slot_save_name(...) and that now only when passwords +// are stored securely: void dlgConnectionProfiles::migrateSecuredPassword(const QString& oldProfile, const QString& newProfile) { const auto& password = character_password_entry->text().trimmed(); @@ -1253,8 +1263,10 @@ void dlgConnectionProfiles::loadSecuredPassword(const QString& profile, L callba if (job->error()) { const auto error = job->errorString(); if (error != QStringLiteral("Entry not found") && error != QStringLiteral("No match")) { - qDebug() << "dlgConnectionProfiles::loadSecuredPassword ERROR: couldn't retrieve secure password for" << profile << ", error is:" << error; + // Report to debug output the problem if it was something other than a couple of 'normal' failure mechanisms: + qDebug().nospace().noquote() << "dlgConnectionProfiles::loadSecuredPassword() ERROR - could not retrieve secure password for \"" << profile << "\", error is: " << error << "."; } + } auto readJob = static_cast(job); diff --git a/src/mudlet.cpp b/src/mudlet.cpp index 9fa3e031a3c..e02ca3d1593 100644 --- a/src/mudlet.cpp +++ b/src/mudlet.cpp @@ -3804,7 +3804,7 @@ void mudlet::slot_password_migrated_to_secure(QKeychain::Job* job) bool mudlet::migratePasswordsToProfileStorage() { if (!mProfilePasswordsToMigrate.isEmpty()) { - qWarning() << "mudlet::migratePasswordsToProfileStorage() warning: password migration is already in progress, won't start another."; + qWarning() << "mudlet::migratePasswordsToProfileStorage() WARMING - password migration is already in progress, so not starting a duplicate action."; return false; } mStorePasswordsSecurely = false; @@ -3838,8 +3838,9 @@ void mudlet::slot_password_migrated_to_profile(QKeychain::Job* job) if (job->error()) { const auto error = job->errorString(); if (error != QStringLiteral("Entry not found") && error != QStringLiteral("No match")) { - qWarning() << "mudlet::slot_password_migrated_to_profile ERROR: couldn't migrate for" << profileName << "; error was:" << error; + qWarning().nospace().noquote() << "mudlet::slot_password_migrated_to_profile(...) ERROR - could not migrate for \"" << profileName << "\"; error was: " << error << "."; } + } else { auto readJob = static_cast(job); writeProfileData(profileName, QStringLiteral("password"), readJob->textData());