Fix multiple TOTP issues, resolves #1360

- Fix crash when deleting TOTP entry - Fix memory leak when selecting TOTP entries - Fix TOTP update timeout on DetailsWidget - Fix TOTP settings attributes not being applied before first call to totpSeed()
keepassxreboot · Jan 22, 2018 · 9035e74 · 9035e74
1 parent 9c641dd
commit 9035e74
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 48 deletions.
diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp
@@ -45,6 +45,7 @@ Entry::Entry()
     m_data.totpStep = Totp::defaultStep;
     m_data.totpDigits = Totp::defaultDigits;
 
+    connect(m_attributes, SIGNAL(modified()), SLOT(updateTotp()));
     connect(m_attributes, SIGNAL(modified()), this, SIGNAL(modified()));
     connect(m_attributes, SIGNAL(defaultKeyModified()), SLOT(emitDataChanged()));
     connect(m_attachments, SIGNAL(modified()), this, SIGNAL(modified()));
@@ -343,9 +344,8 @@ QString Entry::totp() const
         QString output = Totp::generateTotp(seed.toLatin1(), time, m_data.totpDigits, m_data.totpStep);
 
         return QString(output);
-    } else {
-        return QString("");
     }
+    return {};
 }
 
 void Entry::setTotp(const QString& seed, quint8& step, quint8& digits)
@@ -388,23 +388,6 @@ QString Entry::totpSeed() const
         secret = m_attributes->value("TOTP Seed");
     }
 
-    m_data.totpDigits = Totp::defaultDigits;
-    m_data.totpStep = Totp::defaultStep;
-
-    if (m_attributes->hasKey("TOTP Settings")) {
-        // this regex must be kept in sync with the set of allowed short names Totp::shortNameToEncoder
-        QRegularExpression rx(QString("(\\d+);((?:\\d+)|S)"));
-        QRegularExpressionMatch m = rx.match(m_attributes->value("TOTP Settings"));
-        if (m.hasMatch()) {
-            m_data.totpStep = m.captured(1).toUInt();
-            if (Totp::shortNameToEncoder.contains(m.captured(2))) {
-                m_data.totpDigits = Totp::shortNameToEncoder[m.captured(2)];
-            } else {
-                m_data.totpDigits = m.captured(2).toUInt();
-            }
-        }
-    }
-
     return Totp::parseOtpString(secret, m_data.totpDigits, m_data.totpStep);
 }
 
@@ -722,6 +705,33 @@ void Entry::updateModifiedSinceBegin()
     m_modifiedSinceBegin = true;
 }
 
+/**
+ * Update TOTP data whenever entry attributes have changed.
+ */
+void Entry::updateTotp()
+{
+    m_data.totpDigits = Totp::defaultDigits;
+    m_data.totpStep = Totp::defaultStep;
+
+    if (!m_attributes->hasKey("TOTP Settings")) {
+        return;
+    }
+
+    // this regex must be kept in sync with the set of allowed short names Totp::shortNameToEncoder
+    QRegularExpression rx(QString("(\\d+);((?:\\d+)|S)"));
+    QRegularExpressionMatch m = rx.match(m_attributes->value("TOTP Settings"));
+    if (!m.hasMatch()) {
+        return;
+    }
+
+    m_data.totpStep = static_cast<quint8>(m.captured(1).toUInt());
+    if (Totp::shortNameToEncoder.contains(m.captured(2))) {
+        m_data.totpDigits = Totp::shortNameToEncoder[m.captured(2)];
+    } else {
+        m_data.totpDigits = static_cast<quint8>(m.captured(2).toUInt());
+    }
+}
+
 QString Entry::resolveMultiplePlaceholdersRecursive(const QString& str, int maxDepth) const
 {
     if (maxDepth <= 0) {

diff --git a/src/core/Entry.h b/src/core/Entry.h
@@ -210,6 +210,7 @@ private slots:
     void emitDataChanged();
     void updateTimeinfo();
     void updateModifiedSinceBegin();
+    void updateTotp();
 
 private:
     QString resolveMultiplePlaceholdersRecursive(const QString& str, int maxDepth) const;

diff --git a/src/gui/DetailsWidget.cpp b/src/gui/DetailsWidget.cpp
@@ -20,16 +20,12 @@
 #include "ui_DetailsWidget.h"
 
 #include <QDebug>
-#include <QTimer>
 #include <QDir>
 #include <QDesktopServices>
-#include <QTemporaryFile>
 
 #include "core/Config.h"
 #include "core/FilePath.h"
-#include "core/TimeInfo.h"
 #include "gui/Clipboard.h"
-#include "gui/DatabaseWidget.h"
 #include "entry/EntryAttachmentsModel.h"
 
 DetailsWidget::DetailsWidget(QWidget* parent)
@@ -154,13 +150,13 @@ void DetailsWidget::getSelectedEntry(Entry* selectedEntry)
     if (m_currentEntry->hasTotp()) {
         m_step = m_currentEntry->totpStep();
 
-        if (nullptr != m_timer) {
-            m_timer->stop();
+        if (m_timer) {
+            delete m_timer;
         }
-        m_timer = new QTimer(this);
-        connect(m_timer, SIGNAL(timeout()), this, SLOT(updateTotp()));
+        m_timer = new QTimer(selectedEntry);
+        connect(m_timer.data(), SIGNAL(timeout()), this, SLOT(updateTotp()));
         updateTotp();
-        m_timer->start(m_step * 10);
+        m_timer->start(m_step * 1000);
         m_ui->totpButton->show();
     }
 
@@ -288,7 +284,7 @@ void DetailsWidget::updateTotp()
         QString firstHalf = totpCode.left(totpCode.size() / 2);
         QString secondHalf = totpCode.mid(totpCode.size() / 2);
         m_ui->totpLabel->setText(firstHalf + " " + secondHalf);
-    } else if (nullptr != m_timer) {
+    } else if (m_timer.data()) {
         m_timer->stop();
     }
 }

diff --git a/src/gui/DetailsWidget.h b/src/gui/DetailsWidget.h
@@ -20,6 +20,7 @@
 
 #include "gui/DatabaseWidget.h"
 #include <QWidget>
+#include <QTimer>
 
 namespace Ui {
     class DetailsWidget;
@@ -67,7 +68,7 @@ private slots:
     Entry* m_currentEntry;
     Group* m_currentGroup;
     quint8 m_step;
-    QTimer* m_timer;
+    QPointer<QTimer> m_timer = nullptr;
     QWidget* m_attributesTabWidget;
     QWidget* m_attachmentsTabWidget;
     QWidget* m_autotypeTabWidget;

diff --git a/src/gui/TotpDialog.cpp b/src/gui/TotpDialog.cpp
@@ -20,33 +20,24 @@
 #include "ui_TotpDialog.h"
 
 #include "core/Config.h"
-#include "core/Entry.h"
-#include "gui/DatabaseWidget.h"
 #include "gui/Clipboard.h"
 
-#include <QTimer>
-#include <QDateTime>
-#include <QPushButton>
-
 
 TotpDialog::TotpDialog(DatabaseWidget* parent, Entry* entry)
     : QDialog(parent)
     , m_ui(new Ui::TotpDialog())
+    , m_totpUpdateTimer(new QTimer(entry))
+    , m_entry(entry)
 {
-    m_entry = entry;
-    m_parent = parent;
-    m_step = m_entry->totpStep();
-
     m_ui->setupUi(this);
 
+    m_step = m_entry->totpStep();
     uCounter = resetCounter();
     updateProgressBar();
 
-    QTimer* timer = new QTimer(this);
-    connect(timer, SIGNAL(timeout()), this, SLOT(updateProgressBar()));
-    connect(timer, SIGNAL(timeout()), this, SLOT(updateSeconds()));
-    timer->start(m_step * 10);
-
+    connect(m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateProgressBar()));
+    connect(m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateSeconds()));
+    m_totpUpdateTimer->start(m_step * 10);
     updateTotp();
 
     setAttribute(Qt::WA_DeleteOnClose);
@@ -61,7 +52,7 @@ void TotpDialog::copyToClipboard()
 {
     clipboard()->setText(m_entry->totp());
     if (config()->get("MinimizeOnCopy").toBool()) {
-        m_parent->window()->showMinimized();
+        qobject_cast<DatabaseWidget*>(parent())->window()->showMinimized();
     }
 }
 

diff --git a/src/gui/TotpDialog.h b/src/gui/TotpDialog.h
@@ -21,6 +21,8 @@
 
 #include <QDialog>
 #include <QScopedPointer>
+#include <QTimer>
+#include <totp/totp.h>
 #include "core/Entry.h"
 #include "core/Database.h"
 #include "gui/DatabaseWidget.h"
@@ -39,8 +41,9 @@ class TotpDialog : public QDialog
 
 private:
     double uCounter;
-    quint8 m_step;
+    quint8 m_step = Totp::defaultStep;
     QScopedPointer<Ui::TotpDialog> m_ui;
+    QPointer<QTimer> m_totpUpdateTimer;
 
 private Q_SLOTS:
     void updateTotp();
@@ -51,7 +54,6 @@ private Q_SLOTS:
 
 protected:
     Entry* m_entry;
-    DatabaseWidget* m_parent;
 };
 
 #endif // KEEPASSX_TOTPDIALOG_H