Skip to content

Commit

Permalink
Correct issues with TOTP Setup
Browse files Browse the repository at this point in the history 
* Fix #3142 - Warn user when entering invalid TOTP secret key.
* Fix #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
*
  • Loading branch information
@droidmonkey
droidmonkey committed Oct 13, 2019
1 parent ca0c4f5 commit ba502fe
Show file tree
Hide file tree
Showing 7 changed files with 207 additions and 112 deletions.
13 changes: 7 additions & 6 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 Expand Up @@ -762,7 +762,8 @@ Entry* Entry::clone(CloneFlags flags) const
entry->m_autoTypeAssociations->copyDataFrom(m_autoTypeAssociations);
if (flags & CloneIncludeHistory) {
for (Entry* historyItem : m_history) {
Entry* historyItemClone = historyItem->clone(flags & ~CloneIncludeHistory & ~CloneNewUuid & ~CloneResetTimeInfo);
Entry* historyItemClone =
historyItem->clone(flags & ~CloneIncludeHistory & ~CloneNewUuid & ~CloneResetTimeInfo);
historyItemClone->setUpdateTimeinfo(false);
historyItemClone->setUuid(entry->uuid());
historyItemClone->setUpdateTimeinfo(true);
Expand Down
5 changes: 5 additions & 0 deletions src/gui/TotpDialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include "core/Config.h"
#include "gui/Clipboard.h"

#include <QShortcut>

TotpDialog::TotpDialog(QWidget* parent, Entry* entry)
: QDialog(parent)
, m_ui(new Ui::TotpDialog())
Expand All @@ -46,6 +48,9 @@ TotpDialog::TotpDialog(QWidget* parent, Entry* entry)

setAttribute(Qt::WA_DeleteOnClose);

auto shortcut = new QShortcut(QKeySequence(QKeySequence::Copy), this);
connect(shortcut, SIGNAL(activated()), SLOT(copyToClipboard()));

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

connect(m_ui->buttonBox, SIGNAL(rejected()), SLOT(close()));
Expand Down
71 changes: 56 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,90 @@ 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,
"Invalid TOTP Secret",
"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) {
format = settings->format;
if (format == Totp::StorageFormat::LEGACY && m_ui->radioCustom->isChecked()) {
auto answer = MessageBox::question(
this,
"Upgrade TOTP Settings",
"You are currently using legacy TOTP settings which do not fully support custom settings.\n"
"Would you like to upgrade to the latest format?",
MessageBox::Yes | MessageBox::No,
MessageBox::No);
if (answer == MessageBox::Yes) {
format = Totp::DEFAULT_FORMAT;
}
} else if (key.isEmpty()) {
auto answer = MessageBox::question(this,
"Confirm Remove TOTP Settings",
"Are you sure you want to delete TOTP settings for this entry?",
MessageBox::Delete | MessageBox::Cancel);
if (answer == MessageBox::Cancel) {
return;
}
}
}

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
91 changes: 57 additions & 34 deletions src/gui/TotpSetupDialog.ui
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@
<rect>
<x>0</x>
<y>0</y>
<width>249</width>
<height>278</height>
<width>230</width>
<height>240</height>
</rect>
</property>
<property name="sizePolicy">
<sizepolicy hsizetype="Preferred" vsizetype="MinimumExpanding">
<horstretch>0</horstretch>
<verstretch>0</verstretch>
</sizepolicy>
</property>
<property name="windowTitle">
<string>Setup TOTP</string>
</property>
Expand All @@ -25,14 +31,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 @@ -82,7 +100,7 @@
<item>
<widget class="QRadioButton" name="radioCustom">
<property name="text">
<string>Use custom settings</string>
<string>Use custom settings...</string>
</property>
<attribute name="buttonGroup">
<string notr="true">settingsButtonGroup</string>
Expand All @@ -93,13 +111,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 +128,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 +176,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 +213,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
Loading

0 comments on commit ba502fe

Please sign in to comment.