Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CustomData from KDBX4 to Group and Entry #1477

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ set(keepassx_SOURCES
core/AutoTypeMatch.cpp
core/Config.cpp
core/CsvParser.cpp
core/CustomData.cpp
core/Database.cpp
core/DatabaseIcons.cpp
core/Entry.cpp
Expand Down
156 changes: 156 additions & 0 deletions src/core/CustomData.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright (C) 2012 Felix Geyer <debfx@fobos.de>
Copy link
Member

@phoerious phoerious Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this copyright. debfx is the KeePassX maintainer and not involved in KeePassXC.

* Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 or (at your option)
* version 3 of the License.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include "CustomData.h"

CustomData::CustomData(QObject* parent)
: QObject(parent)
{
}

QList<QString> CustomData::keys() const
{
return m_data.keys();
}

bool CustomData::hasKey(const QString& key) const
{
return m_data.contains(key);
}

QString CustomData::value(const QString& key) const
{
return m_data.value(key);
}

bool CustomData::contains(const QString& key) const
{
return m_data.contains(key);
}

bool CustomData::containsValue(const QString& value) const
{
return m_data.values().contains(value);
}

void CustomData::set(const QString& key, const QString& value)
{
bool emitModified = false;

bool addAttribute = !m_data.contains(key);
bool changeValue = !addAttribute && (m_data.value(key) != value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value() will return a default-constructed element if the key does not exist. Avoid that by checking if the key exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case is already covered since !addAttributes equals m_data.contains(key) - m_data.value(key) is only called when the key exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True.


if (addAttribute ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space before )

emit aboutToBeAdded(key);
}

if (addAttribute || changeValue) {
m_data.insert(key, value);
emitModified = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not emit directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I derived the code from an existing class (I think it was EntryAttributes) to reduce style issues. This part was already part of the existing class, so I took it for a code convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just emit the signal right away. There's no point in doing it later.

}

if (emitModified) {
emit modified();
}

if (addAttribute) {
emit added(key);
}
}

void CustomData::remove(const QString& key)
{
emit aboutToBeRemoved(key);

m_data.remove(key);

emit removed(key);
emit modified();
}

void CustomData::rename(const QString& oldKey, const QString& newKey)
{
if (!m_data.contains(oldKey)) {
Q_ASSERT(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid Q_ASSERT(false). Replace with

bool exists = m_data.contains(oldKey);
Q_ASSERT(exists);
if (!exists) {
    return;
]

return;
}

if (m_data.contains(newKey)) {
Q_ASSERT(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. I'm not entirely sure, though, if this is really a condition that should be asserted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the API for CustomData is mainly for plugin developers (at least KeePass provided it mainly for them), it would be nice to show the dev that something unexpected happened like using a key which is already in use.

return;
}

QString data = value(oldKey);

emit aboutToRename(oldKey, newKey);

m_data.remove(oldKey);
m_data.insert(newKey, data);

emit modified();
emit renamed(oldKey, newKey);
}

void CustomData::copyDataFrom(const CustomData* other)
{
if (*this != *other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckieschnick is this really a possibility? shouldn't we assert this doesn't happen instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that familiar with the KeePassXC code. Skimming the code, I think an assert would be correct - but the check it is a pattern in different data classes (EntryAttributes, EntryAttachments) which suggests that there is or there was a case of self assignment (since CustomData are used in Entry, Group and Metadata the risk is even higher). So I pass the question back to you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add an assert in addition to the check. The assert is for debug purposes only.

emit aboutToBeReset();

m_data = other->m_data;

emit reset();
emit modified();
}
}

bool CustomData::operator==(const CustomData& other) const
{
return (m_data == other.m_data);
}

bool CustomData::operator!=(const CustomData& other) const
{
return (m_data != other.m_data);
}

void CustomData::clear()
{
emit aboutToBeReset();

m_data.clear();

emit reset();
emit modified();
}

bool CustomData::isEmpty() const
{
return m_data.isEmpty();
}

int CustomData::dataSize()
{
int size = 0;

QHashIterator<QString, QString> i(m_data);
while (i.hasNext()) {
i.next();
size += i.key().toUtf8().size() + i.value().toUtf8().size();
}
return size;
}
64 changes: 64 additions & 0 deletions src/core/CustomData.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright (C) 2012 Felix Geyer <debfx@fobos.de>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove.

* Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 or (at your option)
* version 3 of the License.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#ifndef KEEPASSX_CUSTOMDATA_H
#define KEEPASSX_CUSTOMDATA_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KEEPASSXC


#include <QMap>
#include <QObject>
#include <QSet>
#include <QStringList>

class CustomData : public QObject
{
Q_OBJECT

public:
explicit CustomData(QObject* parent = nullptr);
QList<QString> keys() const;
bool hasKey(const QString& key) const;
QString value(const QString& key) const;
bool contains(const QString& key) const;
bool containsValue(const QString& value) const;
void set(const QString& key, const QString& value);
void remove(const QString& key);
void rename(const QString& oldKey, const QString& newKey);
void clear();
bool isEmpty() const;
int dataSize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method should be const.

void copyDataFrom(const CustomData* other);
bool operator==(const CustomData& other) const;
bool operator!=(const CustomData& other) const;


signals:
void modified();
void aboutToBeAdded(const QString& key);
void added(const QString& key);
void aboutToBeRemoved(const QString& key);
void removed(const QString& key);
void aboutToRename(const QString& oldKey, const QString& newKey);
void renamed(const QString& oldKey, const QString& newKey);
void aboutToBeReset();
void reset();

private:
QHash<QString, QString> m_data;
};

#endif // KEEPASSX_CUSTOMDATA_H
15 changes: 15 additions & 0 deletions src/core/Entry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Entry::Entry()
: m_attributes(new EntryAttributes(this))
, m_attachments(new EntryAttachments(this))
, m_autoTypeAssociations(new AutoTypeAssociations(this))
, m_customData(new CustomData(this))
, m_tmpHistoryItem(nullptr)
, m_modifiedSinceBegin(false)
, m_updateTimeinfo(true)
Expand All @@ -52,6 +53,7 @@ Entry::Entry()
connect(m_attributes, SIGNAL(defaultKeyModified()), SLOT(emitDataChanged()));
connect(m_attachments, SIGNAL(modified()), this, SIGNAL(modified()));
connect(m_autoTypeAssociations, SIGNAL(modified()), SIGNAL(modified()));
connect(m_customData, SIGNAL(modified()), this, SIGNAL(modified()));

connect(this, SIGNAL(modified()), SLOT(updateTimeinfo()));
connect(this, SIGNAL(modified()), SLOT(updateModifiedSinceBegin()));
Expand Down Expand Up @@ -340,6 +342,16 @@ const EntryAttachments* Entry::attachments() const
return m_attachments;
}

CustomData *Entry::customData()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomData* Entry

{
return m_customData;
}

const CustomData *Entry::customData() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomData* Entry

{
return m_customData;
}

bool Entry::hasTotp() const
{
return m_attributes->hasKey("TOTP Seed") || m_attributes->hasKey("otp");
Expand Down Expand Up @@ -606,6 +618,7 @@ void Entry::truncateHistory()
size += historyItem->attributes()->attributesSize();
size += historyItem->autoTypeAssociations()->associationsSize();
size += historyItem->attachments()->attachmentsSize();
size += historyItem->customData()->dataSize();
const QStringList tags = historyItem->tags().split(delimiter, QString::SkipEmptyParts);
for (const QString& tag : tags) {
size += tag.toUtf8().size();
Expand All @@ -632,6 +645,7 @@ Entry* Entry::clone(CloneFlags flags) const
entry->m_uuid = m_uuid;
}
entry->m_data = m_data;
entry->m_customData->copyDataFrom(m_customData);
entry->m_attributes->copyDataFrom(m_attributes);
entry->m_attachments->copyDataFrom(m_attachments);

Expand Down Expand Up @@ -676,6 +690,7 @@ void Entry::copyDataFrom(const Entry* other)
{
setUpdateTimeinfo(false);
m_data = other->m_data;
m_customData->copyDataFrom(other->m_customData);
m_attributes->copyDataFrom(other->m_attributes);
m_attachments->copyDataFrom(other->m_attachments);
m_autoTypeAssociations->copyDataFrom(other->m_autoTypeAssociations);
Expand Down
5 changes: 5 additions & 0 deletions src/core/Entry.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <QUrl>

#include "core/AutoTypeAssociations.h"
#include "core/CustomData.h"
#include "core/EntryAttachments.h"
#include "core/EntryAttributes.h"
#include "core/TimeInfo.h"
Expand Down Expand Up @@ -107,6 +108,8 @@ class Entry : public QObject
const EntryAttributes* attributes() const;
EntryAttachments* attachments();
const EntryAttachments* attachments() const;
CustomData *customData();
const CustomData *customData() const;
Copy link
Member

@louib louib Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomData* customData. There's more occurrences of this in the PR, I did not comment on all of them.


static const int DefaultIconNumber;
static const int ResolveMaximumDepth;
Expand Down Expand Up @@ -232,6 +235,8 @@ private slots:
EntryAttachments* const m_attachments;
AutoTypeAssociations* const m_autoTypeAssociations;

CustomData* const m_customData;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line above.
All these should probably also be QPointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, why QPointer should be used since these objects are managed by the qt-parent relationship. Do you have some general guideline when to use the one memory management mechanism over the other?

Copy link
Member

@phoerious phoerious Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't confuse QPointer with QSharedPointer or QScopedPointer. QPointer is simply a guard that resets the pointer to nullptr whenever the object behind it is deleted, so we never get dangling pointers. It's a precaution to prevent bugs.
Use QPointers instead of raw pointers for all long-lived pointer variables. QPointers are fully compatible with raw pointers, so you can use them as drop-in replacement without any further code changes.


QList<Entry*> m_history;
Entry* m_tmpHistoryItem;
bool m_modifiedSinceBegin;
Expand Down
Loading