Skip to content

Commit

Permalink
Correct issues with TOTP Setup
Browse files Browse the repository at this point in the history 
* Fix keepassxreboot#3142 - Warn user when entering invalid TOTP secret key.
* Fix keepassxreboot#773 - The TOTP dialog now listens for the copy shortcut without having to press the Copy button.

* Add ability to choose hash algorithm from the TOTP setup dialog
* Add upgrade to "otp" attribute when custom attributes are chosen to prevent data loss

Ran make format
  • Loading branch information
@droidmonkey @scoroi
droidmonkey authored and scoroi committed Nov 10, 2019
1 parent d0f8e65 commit e3f5b4f
Show file tree
Hide file tree
Showing 9 changed files with 210 additions and 121 deletions.
10 changes: 5 additions & 5 deletions src/core/Entry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,16 +442,16 @@ QString Entry::totp() const
void Entry::setTotp(QSharedPointer<Totp::Settings> settings)
{
beginUpdate();
m_attributes->remove(Totp::ATTRIBUTE_OTP);
m_attributes->remove(Totp::ATTRIBUTE_SEED);
m_attributes->remove(Totp::ATTRIBUTE_SETTINGS);

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_data.totpSettings = std::move(settings);

auto text = Totp::writeSettings(m_data.totpSettings, title(), username());
if (m_attributes->hasKey(Totp::ATTRIBUTE_OTP)) {
if (m_data.totpSettings->format != Totp::StorageFormat::LEGACY) {
m_attributes->set(Totp::ATTRIBUTE_OTP, text, true);
} else {
m_attributes->set(Totp::ATTRIBUTE_SEED, m_data.totpSettings->key, true);
Expand Down
10 changes: 8 additions & 2 deletions src/gui/TotpDialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
#include "core/Clock.h"
#include "core/Config.h"
#include "gui/Clipboard.h"
#include "gui/MainWindow.h"

#include <QShortcut>

TotpDialog::TotpDialog(QWidget* parent, Entry* entry)
: QDialog(parent)
Expand All @@ -39,13 +42,16 @@ TotpDialog::TotpDialog(QWidget* parent, Entry* entry)
resetCounter();
updateProgressBar();

connect(parent, SIGNAL(lockedDatabase()), SLOT(close()));
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);

new QShortcut(QKeySequence(QKeySequence::Copy), this, SLOT(copyToClipboard()));

m_ui->buttonBox->button(QDialogButtonBox::Ok)->setText(tr("Copy"));

connect(m_ui->buttonBox, SIGNAL(rejected()), SLOT(close()));
Expand All @@ -61,9 +67,9 @@ void TotpDialog::copyToClipboard()
clipboard()->setText(m_entry->totp());
if (config()->get("HideWindowOnCopy").toBool()) {
if (config()->get("MinimizeOnCopy").toBool()) {
qobject_cast<DatabaseWidget*>(parent())->window()->showMinimized();
getMainWindow()->showMinimized();
} else if (config()->get("DropToBackgroundOnCopy").toBool()) {
qobject_cast<DatabaseWidget*>(parent())->window()->lower();
getMainWindow()->lower();
window()->lower();
}
}
Expand Down
16 changes: 10 additions & 6 deletions src/gui/TotpExportSettingsDialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
*/

#include "TotpExportSettingsDialog.h"

#include "core/Config.h"
#include "core/Entry.h"
#include "gui/Clipboard.h"
#include "gui/DatabaseWidget.h"
#include "gui/MainWindow.h"
#include "gui/SquareSvgWidget.h"
#include "qrcode/QrCode.h"
#include "totp/totp.h"
Expand All @@ -29,6 +31,7 @@
#include <QLabel>
#include <QMessageBox>
#include <QPushButton>
#include <QShortcut>
#include <QSizePolicy>
#include <QVBoxLayout>

Expand All @@ -55,10 +58,13 @@ TotpExportSettingsDialog::TotpExportSettingsDialog(DatabaseWidget* parent, Entry

connect(m_buttonBox, SIGNAL(rejected()), SLOT(close()));
connect(m_buttonBox, SIGNAL(accepted()), SLOT(copyToClipboard()));
connect(m_timer, SIGNAL(timeout()), this, SLOT(autoClose()));
connect(parent, SIGNAL(lockedDatabase()), this, SLOT(close()));
connect(m_timer, SIGNAL(timeout()), SLOT(autoClose()));
connect(parent, SIGNAL(lockedDatabase()), SLOT(close()));

new QShortcut(QKeySequence(QKeySequence::Copy), this, SLOT(copyToClipboard()));

m_buttonBox->button(QDialogButtonBox::Ok)->setText(tr("Copy"));
m_buttonBox->setFocus();
m_countDown->setAlignment(Qt::AlignCenter);

m_secTillClose = 45;
Expand Down Expand Up @@ -92,18 +98,16 @@ TotpExportSettingsDialog::TotpExportSettingsDialog(DatabaseWidget* parent, Entry
errorBox->exec();
close();
}

show();
}

void TotpExportSettingsDialog::copyToClipboard()
{
clipboard()->setText(m_totpUri);
if (config()->get("HideWindowOnCopy").toBool()) {
if (config()->get("MinimizeOnCopy").toBool()) {
static_cast<DatabaseWidget*>(parent())->window()->showMinimized();
getMainWindow()->showMinimized();
} else if (config()->get("DropToBackgroundOnCopy").toBool()) {
static_cast<DatabaseWidget*>(parent())->window()->lower();
getMainWindow()->lower();
window()->lower();
}
}
Expand Down
65 changes: 50 additions & 15 deletions src/gui/TotpSetupDialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "TotpSetupDialog.h"
#include "ui_TotpSetupDialog.h"

#include "core/Base32.h"
#include "gui/MessageBox.h"
#include "totp/totp.h"

TotpSetupDialog::TotpSetupDialog(QWidget* parent, Entry* entry)
Expand All @@ -43,51 +45,84 @@ TotpSetupDialog::~TotpSetupDialog()

void TotpSetupDialog::saveSettings()
{
// Secret key sanity check
auto key = m_ui->seedEdit->text().toLatin1();
auto sanitizedKey = Base32::sanitizeInput(key);
if (sanitizedKey != key) {
MessageBox::information(this,
tr("Invalid TOTP Secret"),
tr("You have entered an invalid secret key. The key must be in Base32 format.\n"
"Example: JBSWY3DPEHPK3PXP"));
return;
}

QString encShortName;
uint digits = Totp::DEFAULT_DIGITS;
uint step = Totp::DEFAULT_STEP;
Totp::Algorithm algorithm = Totp::DEFAULT_ALGORITHM;
Totp::StorageFormat format = Totp::DEFAULT_FORMAT;

if (m_ui->radioSteam->isChecked()) {
digits = Totp::STEAM_DIGITS;
encShortName = Totp::STEAM_SHORTNAME;
} else if (m_ui->radioCustom->isChecked()) {
algorithm = static_cast<Totp::Algorithm>(m_ui->algorithmComboBox->currentData().toInt());
step = m_ui->stepSpinBox->value();
if (m_ui->radio8Digits->isChecked()) {
digits = 8;
} else if (m_ui->radio7Digits->isChecked()) {
digits = 7;
digits = m_ui->digitsSpinBox->value();
}

auto settings = m_entry->totpSettings();
if (settings) {
if (key.isEmpty()) {
auto answer = MessageBox::question(this,
tr("Confirm Remove TOTP Settings"),
tr("Are you sure you want to delete TOTP settings for this entry?"),
MessageBox::Delete | MessageBox::Cancel);
if (answer != MessageBox::Delete) {
return;
}
}

format = settings->format;
if (format == Totp::StorageFormat::LEGACY && m_ui->radioCustom->isChecked()) {
// Implicitly upgrade to the OTPURL format to allow for custom settings
format = Totp::DEFAULT_FORMAT;
}
}

auto settings = Totp::createSettings(
m_ui->seedEdit->text(), digits, step, encShortName, Totp::HashType::Sha1, m_entry->totpSettings());
m_entry->setTotp(settings);
m_entry->setTotp(Totp::createSettings(key, digits, step, format, encShortName, algorithm));
emit totpUpdated();
close();
}

void TotpSetupDialog::toggleCustom(bool status)
{
m_ui->customGroup->setEnabled(status);
m_ui->customSettingsGroup->setEnabled(status);
}

void TotpSetupDialog::init()
{
// Add algorithm choices
auto algorithms = Totp::supportedAlgorithms();
for (const auto& item : algorithms) {
m_ui->algorithmComboBox->addItem(item.first, item.second);
}
m_ui->algorithmComboBox->setCurrentIndex(0);

// Read entry totp settings
auto settings = m_entry->totpSettings();
if (!settings.isNull()) {
if (settings) {
m_ui->seedEdit->setText(settings->key);
m_ui->stepSpinBox->setValue(settings->step);

if (settings->encoder.shortName == Totp::STEAM_SHORTNAME) {
m_ui->radioSteam->setChecked(true);
} else if (settings->custom) {
m_ui->radioCustom->setChecked(true);
if (settings->digits == 8) {
m_ui->radio8Digits->setChecked(true);
} else if (settings->digits == 7) {
m_ui->radio7Digits->setChecked(true);
} else {
m_ui->radio6Digits->setChecked(true);
m_ui->digitsSpinBox->setValue(settings->digits);
int index = m_ui->algorithmComboBox->findData(settings->algorithm);
if (index != -1) {
m_ui->algorithmComboBox->setCurrentIndex(index);
}
}
}
Expand Down
79 changes: 48 additions & 31 deletions src/gui/TotpSetupDialog.ui
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,26 @@
<item>
<widget class="QLabel" name="label_3">
<property name="text">
<string>Key:</string>
<string>Secret Key:</string>
</property>
</widget>
</item>
<item>
<widget class="QLineEdit" name="seedEdit">
<property name="sizePolicy">
<sizepolicy hsizetype="MinimumExpanding" vsizetype="Fixed">
<horstretch>0</horstretch>
<verstretch>0</verstretch>
</sizepolicy>
</property>
<property name="minimumSize">
<size>
<width>150</width>
<height>0</height>
</size>
</property>
<property name="toolTip">
<string>Secret key in Base32 format</string>
<string>Secret key must be in Base32 format</string>
</property>
<property name="accessibleName">
<string>Secret key field</string>
Expand Down Expand Up @@ -93,13 +105,10 @@
</widget>
</item>
<item>
<widget class="QGroupBox" name="customGroup">
<widget class="QGroupBox" name="customSettingsGroup">
<property name="enabled">
<bool>false</bool>
</property>
<property name="styleSheet">
<string notr="true"/>
</property>
<property name="title">
<string>Custom Settings</string>
</property>
Expand All @@ -113,20 +122,36 @@
<property name="labelAlignment">
<set>Qt::AlignRight|Qt::AlignTop|Qt::AlignTrailing</set>
</property>
<property name="horizontalSpacing">
<number>7</number>
</property>
<property name="verticalSpacing">
<number>7</number>
</property>
<property name="leftMargin">
<number>5</number>
<number>20</number>
</property>
<property name="rightMargin">
<number>5</number>
<number>20</number>
</property>
<item row="1" column="0">
<widget class="QLabel" name="algorithmLabel">
<property name="text">
<string>Algorithm:</string>
</property>
</widget>
</item>
<item row="1" column="1">
<widget class="QComboBox" name="algorithmComboBox"/>
</item>
<item row="2" column="0">
<widget class="QLabel" name="stepLabel">
<property name="text">
<string>Time step:</string>
</property>
</widget>
</item>
<item row="1" column="1">
<item row="2" column="1">
<widget class="QSpinBox" name="stepSpinBox">
<property name="accessibleName">
<string>Time step field</string>
Expand All @@ -145,34 +170,26 @@
</property>
</widget>
</item>
<item row="2" column="0">
<item row="3" column="0">
<widget class="QLabel" name="digitsLabel">
<property name="text">
<string>Code size:</string>
</property>
</widget>
</item>
<item row="2" column="1">
<widget class="QRadioButton" name="radio6Digits">
<property name="text">
<string>6 digits</string>
<item row="3" column="1">
<widget class="QSpinBox" name="digitsSpinBox">
<property name="suffix">
<string> digits</string>
</property>
<property name="checked">
<bool>true</bool>
<property name="minimum">
<number>6</number>
</property>
</widget>
</item>
<item row="3" column="1">
<widget class="QRadioButton" name="radio7Digits">
<property name="text">
<string>7 digits</string>
<property name="maximum">
<number>10</number>
</property>
</widget>
</item>
<item row="4" column="1">
<widget class="QRadioButton" name="radio8Digits">
<property name="text">
<string>8 digits</string>
<property name="singleStep">
<number>1</number>
</property>
</widget>
</item>
Expand All @@ -190,16 +207,16 @@
</widget>
</item>
</layout>
<zorder>customSettingsGroup</zorder>
<zorder>buttonBox</zorder>
<zorder>groupBox</zorder>
</widget>
<tabstops>
<tabstop>seedEdit</tabstop>
<tabstop>radioDefault</tabstop>
<tabstop>radioSteam</tabstop>
<tabstop>radioCustom</tabstop>
<tabstop>stepSpinBox</tabstop>
<tabstop>radio6Digits</tabstop>
<tabstop>radio7Digits</tabstop>
<tabstop>radio8Digits</tabstop>
</tabstops>
<resources/>
<connections/>
Expand Down
5 changes: 3 additions & 2 deletions src/gui/entry/EditEntryWidget.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -962,8 +962,9 @@ void EditEntryWidget::setForms(Entry* entry, bool restore)

#ifdef WITH_XC_BROWSER
if (m_customData->contains(BrowserService::OPTION_SKIP_AUTO_SUBMIT)) {
m_browserUi->skipAutoSubmitCheckbox->setChecked(m_customData->value(BrowserService::OPTION_SKIP_AUTO_SUBMIT)
== "true");
// clang-format off
m_browserUi->skipAutoSubmitCheckbox->setChecked(m_customData->value(BrowserService::OPTION_SKIP_AUTO_SUBMIT) == "true");
// clang-format on
} else {
m_browserUi->skipAutoSubmitCheckbox->setChecked(false);
}
Expand Down
Loading

0 comments on commit e3f5b4f

Please sign in to comment.