Skip to content

Commit

Permalink
Merge pull request #5660 from Aetf/fix/fdosecrets-5279
Browse files Browse the repository at this point in the history 
Secret Service: cleanup and fix crash
  • Loading branch information
@Aetf
Aetf authored Nov 16, 2020
2 parents f8f2271 + 9f41189 commit 3d10f31
Show file tree
Hide file tree
Showing 21 changed files with 562 additions and 228 deletions.
16 changes: 8 additions & 8 deletions src/fdosecrets/FdoSecretsPlugin.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@

#include <QFile>

#include <utility>

using FdoSecrets::Service;

// TODO: Only used for testing. Need to split service functions away from settings page.
Expand Down Expand Up @@ -65,12 +63,8 @@ void FdoSecretsPlugin::updateServiceState()
{
if (FdoSecrets::settings()->isEnabled()) {
if (!m_secretService && m_dbTabs) {
m_secretService.reset(new Service(this, m_dbTabs));
connect(m_secretService.data(), &Service::error, this, [this](const QString& msg) {
emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
});
if (!m_secretService->initialize()) {
m_secretService.reset();
m_secretService = Service::Create(this, m_dbTabs);
if (!m_secretService) {
FdoSecrets::settings()->setEnabled(false);
return;
}
Expand Down Expand Up @@ -107,6 +101,12 @@ void FdoSecretsPlugin::emitRequestShowNotification(const QString& msg, const QSt
emit requestShowNotification(msg, title, 10000);
}

void FdoSecretsPlugin::emitError(const QString& msg)
{
emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
qDebug() << msg;
}

QString FdoSecretsPlugin::reportExistingService() const
{
auto pidStr = tr("Unknown", "Unknown PID");
Expand Down
11 changes: 9 additions & 2 deletions src/fdosecrets/FdoSecretsPlugin.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
#include "gui/Icons.h"

#include <QPointer>
#include <QScopedPointer>

#include <memory>

class DatabaseTabWidget;

Expand Down Expand Up @@ -77,6 +78,12 @@ public slots:
void emitRequestSwitchToDatabases();
void emitRequestShowNotification(const QString& msg, const QString& title = {});

/**
* @brief Show error in the GUI
* @param msg
*/
void emitError(const QString& msg);

signals:
void error(const QString& msg);
void requestSwitchToDatabases();
Expand All @@ -86,7 +93,7 @@ public slots:

private:
QPointer<DatabaseTabWidget> m_dbTabs;
QScopedPointer<FdoSecrets::Service> m_secretService;
QSharedPointer<FdoSecrets::Service> m_secretService;
};

#endif // KEEPASSXC_FDOSECRETSPLUGIN_H
114 changes: 66 additions & 48 deletions src/fdosecrets/objects/Collection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@

#include "Collection.h"

#include "fdosecrets/FdoSecretsPlugin.h"
#include "fdosecrets/FdoSecretsSettings.h"
#include "fdosecrets/objects/Item.h"
#include "fdosecrets/objects/Prompt.h"
#include "fdosecrets/objects/Service.h"
#include "fdosecrets/objects/Session.h"

#include "core/Config.h"
#include "core/Database.h"
#include "core/Tools.h"
#include "gui/DatabaseTabWidget.h"
#include "gui/DatabaseWidget.h"
Expand All @@ -34,16 +34,19 @@

namespace FdoSecrets
{
Collection* Collection::Create(Service* parent, DatabaseWidget* backend)
{
return new Collection(parent, backend);
}

Collection::Collection(Service* parent, DatabaseWidget* backend)
: DBusObject(parent)
: DBusObjectHelper(parent)
, m_backend(backend)
, m_exposedGroup(nullptr)
, m_registered(false)
{
// whenever the file path or the database object itself change, we do a full reload.
connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackend);
connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackend);
connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackendOrDelete);
connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackendOrDelete);

// also remember to clear/populate the database when lock state changes.
connect(backend, &DatabaseWidget::databaseUnlocked, this, &Collection::onDatabaseLockChanged);
Expand All @@ -56,37 +59,36 @@ namespace FdoSecrets
}
emit doneUnlockCollection(accepted);
});

reloadBackend();
}

void Collection::reloadBackend()
bool Collection::reloadBackend()
{
if (m_registered) {
// delete all items
// this has to be done because the backend is actually still there, just we don't expose them
// NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items.
while (!m_items.isEmpty()) {
m_items.first()->doDelete();
}
cleanupConnections();
Q_ASSERT(m_backend);

unregisterCurrentPath();
m_registered = false;
// delete all items
// this has to be done because the backend is actually still there, just we don't expose them
// NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items.
while (!m_items.isEmpty()) {
m_items.first()->doDelete();
}

Q_ASSERT(m_backend);
cleanupConnections();
unregisterPrimaryPath();

// make sure we have updated copy of the filepath, which is used to identify the database.
m_backendPath = m_backend->database()->filePath();

// the database may not have a name (derived from filePath) yet, which may happen if it's newly created.
// defer the registration to next time a file path change happens.
if (!name().isEmpty()) {
registerWithPath(
QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name())),
new CollectionAdaptor(this));
m_registered = true;
m_backendPath = m_backend->database()->canonicalFilePath();

// register the object, handling potentially duplicated name
auto name = encodePath(this->name());
auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), name);
if (!registerWithPath(path)) {
// try again with a suffix
name += QStringLiteral("_%1").arg(Tools::uuidToHex(QUuid::createUuid()).left(4));
path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), name);

if (!registerWithPath(path)) {
service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name));
return false;
}
}

// populate contents after expose on dbus, because items rely on parent's dbus object path
Expand All @@ -95,6 +97,15 @@ namespace FdoSecrets
} else {
cleanupConnections();
}

return true;
}

void Collection::reloadBackendOrDelete()
{
if (!reloadBackend()) {
doDelete();
}
}

DBusReturn<void> Collection::ensureBackend() const
Expand Down Expand Up @@ -197,7 +208,11 @@ namespace FdoSecrets
}

// Delete means close database
auto prompt = new DeleteCollectionPrompt(service(), this);
auto dpret = DeleteCollectionPrompt::Create(service(), this);
if (dpret.isError()) {
return dpret;
}
auto prompt = dpret.value();
if (backendLocked()) {
// this won't raise a dialog, immediate execute
auto pret = prompt->prompt({});
Expand Down Expand Up @@ -311,12 +326,12 @@ namespace FdoSecrets
itemPath = attributes.value(ItemAttributes::PathKey);

// check existing item using attributes
auto existings = searchItems(attributes);
if (existings.isError()) {
return existings;
auto existing = searchItems(attributes);
if (existing.isError()) {
return existing;
}
if (!existings.value().isEmpty() && replace) {
item = existings.value().front();
if (!existing.value().isEmpty() && replace) {
item = existing.value().front();
newlyCreated = false;
}
}
Expand Down Expand Up @@ -400,11 +415,13 @@ namespace FdoSecrets

emit aliasAboutToAdd(alias);

bool ok = QDBusConnection::sessionBus().registerObject(
QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), this);
bool ok =
registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), false);
if (ok) {
m_aliases.insert(alias);
emit aliasAdded(alias);
} else {
return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
}

return {};
Expand Down Expand Up @@ -435,16 +452,15 @@ namespace FdoSecrets
QString Collection::name() const
{
if (m_backendPath.isEmpty()) {
// This is a newly created db without saving to file.
// This name is also used to register dbus path.
// For simplicity, we don't monitor the name change.
// So the dbus object path is not updated if the db name
// changes. This should not be a problem because once the database
// gets saved, the dbus path will be updated to use filename and
// everything back to normal.
// This is a newly created db without saving to file,
// but we have to give a name, which is used to register dbus path.
// We use database name for this purpose. For simplicity, we don't monitor the name change.
// So the dbus object path is not updated if the db name changes.
// This should not be a problem because once the database gets saved,
// the dbus path will be updated to use filename and everything back to normal.
return m_backend->database()->metadata()->name();
}
return QFileInfo(m_backendPath).baseName();
return QFileInfo(m_backendPath).completeBaseName();
}

DatabaseWidget* Collection::backend() const
Expand All @@ -466,7 +482,7 @@ namespace FdoSecrets

void Collection::populateContents()
{
if (!m_registered) {
if (!m_backend) {
return;
}

Expand Down Expand Up @@ -558,7 +574,7 @@ namespace FdoSecrets
return;
}

auto item = new Item(this, entry);
auto item = Item::Create(this, entry);
m_items << item;
m_entryToItem[entry] = item;

Expand Down Expand Up @@ -625,7 +641,7 @@ namespace FdoSecrets

emit collectionAboutToDelete();

unregisterCurrentPath();
unregisterPrimaryPath();

// remove alias manually to trigger signal
for (const auto& a : aliases()) {
Expand All @@ -643,6 +659,8 @@ namespace FdoSecrets
// reset backend and delete self
m_backend = nullptr;
deleteLater();

// items will be removed automatically as they are children objects
}

void Collection::cleanupConnections()
Expand Down
28 changes: 21 additions & 7 deletions src/fdosecrets/objects/Collection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,24 @@ namespace FdoSecrets
class Item;
class PromptBase;
class Service;
class Collection : public DBusObject
class Collection : public DBusObjectHelper<Collection, CollectionAdaptor>
{
Q_OBJECT
public:

explicit Collection(Service* parent, DatabaseWidget* backend);

public:
/**
* @brief Create a new instance of `Collection`
* @param parent the owning Service
* @param backend the widget containing the database
* @return pointer to created instance, or nullptr when error happens.
* This may be caused by
* - DBus path registration error
* - database has no exposed group
*/
static Collection* Create(Service* parent, DatabaseWidget* backend);

DBusReturn<const QList<Item*>> items() const;

DBusReturn<QString> label() const;
Expand Down Expand Up @@ -101,7 +113,7 @@ namespace FdoSecrets
static EntrySearcher::SearchTerm attributeToTerm(const QString& key, const QString& value);

public slots:
// expose some methods for Prmopt to use
// expose some methods for Prompt to use
bool doLock();
void doUnlock();
// will remove self
Expand All @@ -110,11 +122,15 @@ namespace FdoSecrets
// delete the Entry in backend from this collection
void doDeleteEntries(QList<Entry*> entries);

// force reload info from backend, potentially delete self
bool reloadBackend();

private slots:
void onDatabaseLockChanged();
void onDatabaseExposedGroupChanged();
// force reload info from backend, potentially delete self
void reloadBackend();

// calls reloadBackend, delete self when error
void reloadBackendOrDelete();

private:
friend class DeleteCollectionPrompt;
Expand Down Expand Up @@ -154,8 +170,6 @@ namespace FdoSecrets
QSet<QString> m_aliases;
QList<Item*> m_items;
QMap<const Entry*, Item*> m_entryToItem;

bool m_registered;
};

} // namespace FdoSecrets
Expand Down
Loading

0 comments on commit 3d10f31

Please sign in to comment.