Fix issues with TOTP

* otp setting is properly loaded and saved (Fix #2671) * Removing the key from TOTP Setup clears all TOTP settings for entry
keepassxreboot · Feb 2, 2019 · c2f35d7 · c2f35d7
1 parent af41511
commit c2f35d7
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 30 deletions.
diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp
@@ -427,14 +427,21 @@ QString Entry::totp() const
 void Entry::setTotp(QSharedPointer<Totp::Settings> settings)
 {
     beginUpdate();
-    m_data.totpSettings = std::move(settings);
-
-    auto text = Totp::writeSettings(m_data.totpSettings, title(), username());
-    if (m_attributes->hasKey(Totp::ATTRIBUTE_OTP)) {
-        m_attributes->set(Totp::ATTRIBUTE_OTP, text, true);
+    if (settings->key.isEmpty()) {
+        m_data.totpSettings.reset();
+        m_attributes->remove(Totp::ATTRIBUTE_OTP);
+        m_attributes->remove(Totp::ATTRIBUTE_SEED);
+        m_attributes->remove(Totp::ATTRIBUTE_SETTINGS);
     } else {
-        m_attributes->set(Totp::ATTRIBUTE_SEED, m_data.totpSettings->key, true);
-        m_attributes->set(Totp::ATTRIBUTE_SETTINGS, text);
+        m_data.totpSettings = std::move(settings);
+
+        auto text = Totp::writeSettings(m_data.totpSettings, title(), username());
+        if (m_attributes->hasKey(Totp::ATTRIBUTE_OTP)) {
+            m_attributes->set(Totp::ATTRIBUTE_OTP, text, true);
+        } else {
+            m_attributes->set(Totp::ATTRIBUTE_SEED, m_data.totpSettings->key, true);
+            m_attributes->set(Totp::ATTRIBUTE_SETTINGS, text);
+        }
     }
     endUpdate();
 }

diff --git a/src/core/EntryAttributes.cpp b/src/core/EntryAttributes.cpp
@@ -137,7 +137,6 @@ void EntryAttributes::remove(const QString& key)
     Q_ASSERT(!isDefaultAttribute(key));
 
     if (!m_attributes.contains(key)) {
-        Q_ASSERT(false);
         return;
     }
 

diff --git a/src/gui/TotpSetupDialog.cpp b/src/gui/TotpSetupDialog.cpp
@@ -45,16 +45,21 @@ void TotpSetupDialog::saveSettings()
 {
     QString encShortName;
     uint digits = Totp::DEFAULT_DIGITS;
-    if (m_ui->radio8Digits->isChecked()) {
-        digits = 8;
-    } else if (m_ui->radio7Digits->isChecked()) {
-        digits = 7;
-    } else if (m_ui->radioSteam->isChecked()) {
+    uint step = Totp::DEFAULT_STEP;
+
+    if (m_ui->radioSteam->isChecked()) {
         digits = Totp::STEAM_DIGITS;
         encShortName = Totp::STEAM_SHORTNAME;
+    } else if (m_ui->radioCustom->isChecked()) {
+        step = m_ui->stepSpinBox->value();
+        if (m_ui->radio8Digits->isChecked()) {
+            digits = 8;
+        } else if (m_ui->radio7Digits->isChecked()) {
+            digits = 7;
+        }
     }
 
-    auto settings = Totp::createSettings(m_ui->seedEdit->text(), digits, m_ui->stepSpinBox->value(), encShortName);
+    auto settings = Totp::createSettings(m_ui->seedEdit->text(), digits, step, encShortName, m_entry->totpSettings());
     m_entry->setTotp(settings);
     emit totpUpdated();
     close();

diff --git a/src/totp/totp.cpp b/src/totp/totp.cpp
@@ -55,10 +55,16 @@ QSharedPointer<Totp::Settings> Totp::parseSettings(const QString& rawSettings, c
         QUrlQuery query(rawSettings);
         if (query.hasQueryItem("key")) {
             // Compatibility with "KeeOtp" plugin
-            // if settings are changed, will convert to semi-colon format
+            settings->keeOtp = true;
             settings->key = query.queryItemValue("key");
-            settings->digits = query.queryItemValue("size").toUInt();
-            settings->step = query.queryItemValue("step").toUInt();
+            settings->digits = DEFAULT_DIGITS;
+            settings->step = DEFAULT_STEP;
+            if (query.hasQueryItem("size")) {
+                settings->digits = query.queryItemValue("size").toUInt();
+            }
+            if (query.hasQueryItem("step")) {
+                settings->step = query.queryItemValue("step").toUInt();
+            }
         } else {
             // Parse semi-colon separated values ([step];[digits|S])
             auto vars = rawSettings.split(";");
@@ -88,12 +94,24 @@ QSharedPointer<Totp::Settings> Totp::parseSettings(const QString& rawSettings, c
     return settings;
 }
 
-QSharedPointer<Totp::Settings>
-Totp::createSettings(const QString& key, const uint digits, const uint step, const QString& encoderShortName)
+QSharedPointer<Totp::Settings> Totp::createSettings(const QString& key,
+                                                    const uint digits,
+                                                    const uint step,
+                                                    const QString& encoderShortName,
+                                                    QSharedPointer<Totp::Settings> prevSettings)
 {
     bool isCustom = digits != DEFAULT_DIGITS || step != DEFAULT_STEP;
-    return QSharedPointer<Totp::Settings>(
-        new Totp::Settings{getEncoderByShortName(encoderShortName), key, false, isCustom, digits, step});
+    if (prevSettings) {
+        prevSettings->key = key;
+        prevSettings->digits = digits;
+        prevSettings->step = step;
+        prevSettings->encoder = Totp::getEncoderByShortName(encoderShortName);
+        prevSettings->custom = isCustom;
+        return prevSettings;
+    } else {
+        return QSharedPointer<Totp::Settings>(
+            new Totp::Settings{getEncoderByShortName(encoderShortName), key, false, false, isCustom, digits, step});
+    }
 }
 
 QString Totp::writeSettings(const QSharedPointer<Totp::Settings>& settings,
@@ -118,15 +136,16 @@ QString Totp::writeSettings(const QSharedPointer<Totp::Settings>& settings,
             urlstring.append("&encoder=").append(settings->encoder.name);
         }
         return urlstring;
-    }
-
-    // Semicolon output [step];[encoder]
-    if (!settings->encoder.shortName.isEmpty()) {
+    } else if (settings->keeOtp) {
+        // KeeOtp output
+        return QString("key=%1&size=%2&step=%3").arg(settings->key).arg(settings->digits).arg(settings->step);
+    } else if (!settings->encoder.shortName.isEmpty()) {
+        // Semicolon output [step];[encoder]
         return QString("%1;%2").arg(settings->step).arg(settings->encoder.shortName);
+    } else {
+        // Semicolon output [step];[digits]
+        return QString("%1;%2").arg(settings->step).arg(settings->digits);
     }
-
-    // Semicolon output [step];[digits]
-    return QString("%1;%2").arg(settings->step).arg(settings->digits);
 }
 
 QString Totp::generateTotp(const QSharedPointer<Totp::Settings>& settings, const quint64 time)

diff --git a/src/totp/totp.h b/src/totp/totp.h
@@ -44,6 +44,7 @@ namespace Totp
         Totp::Encoder encoder;
         QString key;
         bool otpUrl;
+        bool keeOtp;
         bool custom;
         uint digits;
         uint step;
@@ -59,8 +60,11 @@ namespace Totp
     static const QString ATTRIBUTE_SETTINGS = "TOTP Settings";
 
     QSharedPointer<Totp::Settings> parseSettings(const QString& rawSettings, const QString& key = {});
-    QSharedPointer<Totp::Settings>
-    createSettings(const QString& key, const uint digits, const uint step, const QString& encoderShortName = {});
+    QSharedPointer<Totp::Settings> createSettings(const QString& key,
+                                                  const uint digits,
+                                                  const uint step,
+                                                  const QString& encoderShortName = {},
+                                                  QSharedPointer<Totp::Settings> prevSettings = {});
     QString writeSettings(const QSharedPointer<Totp::Settings>& settings,
                           const QString& title = {},
                           const QString& username = {},