sean-m
/
keysmith
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

chore: perform some code cleanups suggested by clazy 

These cleanups block the invalid password detection/retry UX feature from landing.

See-Also: https://invent.kde.org/utilities/keysmith/-/merge_requests/71
master
Johan Ouwerkerk 2020-11-23 19:24:26 +01:00
parent 4ca180abf1
commit c31aa8df52
5 changed files with 92 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
autotests/account/file-jobs/request-account-password.cpp
View File

@ -13,11 +13,11 @@
#include <QTest>
#include <QtDebug>
static QString existingPasswordIniResource(QLatin1String(":/request-account-password/existing-password.ini"));
static QString newPasswordIniResource(QLatin1String(":/request-account-password/new-password.ini"));
static QString newPasswordIniResultResource(QLatin1String(":/request-account-password/new-password-result.ini"));
static QString existingPasswordIniResource(QStringLiteral(":/request-account-password/existing-password.ini"));
static QString newPasswordIniResource(QStringLiteral(":/request-account-password/new-password.ini"));
static QString newPasswordIniResultResource(QStringLiteral(":/request-account-password/new-password-result.ini"));
class RequestAccountPasswordTest: public QObject
class RequestAccountPasswordTest: public QObject // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
private Q_SLOTS:
@ -30,7 +30,7 @@ private Q_SLOTS:
void RequestAccountPasswordTest::testAbortBeforeRun(void)
{
const QString isolated(QLatin1String("abort-before-run.ini"));
const QString isolated(QStringLiteral("abort-before-run.ini"));
QVERIFY2(test::copyResourceAsWritable(newPasswordIniResource, isolated), "accounts INI resource should be available as file");
int openCounter = 0;
@ -78,7 +78,7 @@ void RequestAccountPasswordTest::testAbortBeforeRun(void)
void RequestAccountPasswordTest::testNewPassword(void)
{
const QString isolated(QLatin1String("supply-new-password.ini"));
const QString isolated(QStringLiteral("supply-new-password.ini"));
QVERIFY2(test::copyResourceAsWritable(newPasswordIniResource, isolated), "accounts INI resource should be available as file");
int openCounter = 0;
@ -112,7 +112,7 @@ void RequestAccountPasswordTest::testNewPassword(void)
QCOMPARE(unlocked.count(), 0);
QCOMPARE(jobFinished.count(), 0);
QString password(QLatin1String("hello, world"));
QString password(QStringLiteral("hello, world"));
std::optional<secrets::KeyDerivationParameters> defaults = secrets::KeyDerivationParameters::create();
QVERIFY2(defaults, "should be able to construct default key derivation parameters");
QVERIFY2(secret.answerNewPassword(password, *defaults), "should be able to answer (new) password");
@ -140,7 +140,7 @@ void RequestAccountPasswordTest::testNewPassword(void)
void RequestAccountPasswordTest::testNewPasswordAbort(void)
{
const QString isolated(QLatin1String("abort-new-password.ini"));
const QString isolated(QStringLiteral("abort-new-password.ini"));
QVERIFY2(test::copyResourceAsWritable(newPasswordIniResource, isolated), "accounts INI resource should be available as file");
int openCounter = 0;
@ -196,7 +196,7 @@ void RequestAccountPasswordTest::testNewPasswordAbort(void)
void RequestAccountPasswordTest::testExistingPassword(void)
{
const QString isolated(QLatin1String("supply-existing-password.ini"));
const QString isolated(QStringLiteral("supply-existing-password.ini"));
QVERIFY2(test::copyResourceAsWritable(existingPasswordIniResource, isolated), "accounts INI resource should be available as file");
int openCounter = 0;
@ -230,7 +230,7 @@ void RequestAccountPasswordTest::testExistingPassword(void)
QCOMPARE(unlocked.count(), 0);
QCOMPARE(jobFinished.count(), 0);
QString password(QLatin1String("hello, world"));
QString password(QStringLiteral("hello, world"));
QVERIFY2(secret.answerExistingPassword(password), "should be able to answer (existing) password");
QVERIFY2(test::signal_eventually_emitted_once(passwordAvailable), "(existing) password should be accepted");
@ -256,7 +256,7 @@ void RequestAccountPasswordTest::testExistingPassword(void)
void RequestAccountPasswordTest::testExistingPasswordAbort(void)
{
const QString isolated(QLatin1String("abort-existing-password.ini"));
const QString isolated(QStringLiteral("abort-existing-password.ini"));
QVERIFY2(test::copyResourceAsWritable(existingPasswordIniResource, isolated), "accounts INI resource should be available as file");
int openCounter = 0;

10
autotests/account/keys/account-secret-password-flow.cpp
View File

@ -9,7 +9,7 @@
#include <QSignalSpy>
#include <QTest>
class PasswordFlowTest : public QObject
class PasswordFlowTest : public QObject // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
private Q_SLOTS:
@ -67,12 +67,12 @@ void PasswordFlowTest::supplyNewPassword(void)
QCOMPARE(requestsCancelled.count(), 0);
// advance the state: supply password
QString password(QLatin1String("hello, world"));
QString password(QStringLiteral("hello, world"));
QVERIFY2(m_keyParams, "should be able to construct key derivation parameters");
QVERIFY2(uut.answerNewPassword(password, *m_keyParams), "(new) password should be accepted");
QVERIFY2(test::signal_eventually_emitted_once(passwordAvailable), "availability of the (new) password should be signalled");
QCOMPARE(password, QString(QLatin1String("************")));
QCOMPARE(password, QStringLiteral("************"));
// check the state is correctly updated
QCOMPARE(uut.isStillAlive(), true);
@ -251,11 +251,11 @@ void PasswordFlowTest::supplyExistingPassword(void)
QCOMPARE(requestsCancelled.count(), 0);
// advance the state: supply password
QString password(QLatin1String("hello, world"));
QString password(QStringLiteral("hello, world"));
QVERIFY2(uut.answerExistingPassword(password), "(existing) password should be accepted");
QVERIFY2(test::signal_eventually_emitted_once(passwordAvailable), "availability of the (existing) password should be signalled");
QCOMPARE(password, QString(QLatin1String("************")));
QCOMPARE(password, QStringLiteral("************"));
// check the state is correctly updated
QCOMPARE(uut.isStillAlive(), true);

2
autotests/account/test-utils/secret.cpp
View File

@ -17,7 +17,7 @@ namespace test
QByteArray salt;
salt.resize(crypto_pwhash_SALTBYTES);
salt.fill('\x0', -1);
QString password(QLatin1String("password"));
QString password(QStringLiteral("password"));
return useDummyPassword(secret, password, salt);
}

120
src/account/actions_p.cpp
View File

@ -18,12 +18,8 @@
KEYSMITH_LOGGER(logger, ".accounts.actions")
KEYSMITH_LOGGER(dispatcherLogger, ".accounts.dispatcher")
static const QMetaEnum hashEnum = QMetaEnum::fromType<accounts::Account::Hash>();
static const QVariant hashDefault = QVariant::fromValue<accounts::Account::Hash>(accounts::Account::Sha1);
static const QString trueVariantValue(QLatin1String("true"));
static const QString falseVariantValue(QLatin1String("false"));
static const quint64 maxCounter = std::numeric_limits<quint64>::max();
static const int hashTypeId = qRegisterMetaType<accounts::Account::Hash>();
namespace accounts
{
@ -68,16 +64,16 @@ namespace accounts
}
SaveHotp::SaveHotp(const SettingsProvider &settings,
const QUuid &id, const QString &accountName, const QString &issuer,
const QUuid id, const QString &accountName, const QString &issuer,
const secrets::EncryptedSecret &secret, uint tokenLength,
quint64 counter, const std::optional<uint> &offset, bool checksum) :
quint64 counter, const std::optional<uint> offset, bool checksum) :
AccountJob(), m_settings(settings), m_id(id), m_accountName(accountName), m_issuer(issuer),
m_secret(secret), m_tokenLength(tokenLength), m_counter(counter), m_offset(offset), m_checksum(checksum)
{
}
SaveTotp::SaveTotp(const SettingsProvider &settings,
const QUuid &id, const QString &accountName, const QString &issuer,
const QUuid id, const QString &accountName, const QString &issuer,
const secrets::EncryptedSecret &secret, uint tokenLength,
uint timeStep, const QDateTime &epoch, Account::Hash hash,
const std::function<qint64(void)> &clock) :
@ -113,21 +109,21 @@ namespace accounts
const QString group = m_id.toString();
settings.remove(group);
settings.beginGroup(group);
settings.setValue("account", m_accountName);
settings.setValue(QStringLiteral("account"), m_accountName);
if (!m_issuer.isNull()) {
settings.setValue("issuer", m_issuer);
settings.setValue(QStringLiteral("issuer"), m_issuer);
}
settings.setValue("type", "hotp");
settings.setValue(QStringLiteral("type"), QStringLiteral("hotp"));
QString encodedNonce = QString::fromUtf8(m_secret.nonce().toBase64(QByteArray::Base64Encoding));
QString encodedSecret = QString::fromUtf8(m_secret.cryptText().toBase64(QByteArray::Base64Encoding));
settings.setValue("secret", encodedSecret);
settings.setValue("nonce", encodedNonce);
settings.setValue("counter", m_counter);
settings.setValue("pinLength", m_tokenLength);
settings.setValue(QStringLiteral("secret"), encodedSecret);
settings.setValue(QStringLiteral("nonce"), encodedNonce);
settings.setValue(QStringLiteral("counter"), m_counter);
settings.setValue(QStringLiteral("pinLength"), m_tokenLength);
if (m_offset) {
settings.setValue("offset", *m_offset);
settings.setValue(QStringLiteral("offset"), *m_offset);
}
settings.setValue("checksum", m_checksum);
settings.setValue(QStringLiteral("checksum"), m_checksum);
settings.endGroup();
// Try to guarantee that data will have been written before claiming the account was actually saved
@ -168,19 +164,19 @@ namespace accounts
const QString group = m_id.toString();
settings.remove(group);
settings.beginGroup(group);
settings.setValue("account", m_accountName);
settings.setValue(QStringLiteral("account"), m_accountName);
if (!m_issuer.isNull()) {
settings.setValue("issuer", m_issuer);
settings.setValue(QStringLiteral("issuer"), m_issuer);
}
settings.setValue("type", "totp");
settings.setValue(QStringLiteral("type"), QStringLiteral("totp"));
QString encodedNonce = QString::fromUtf8(m_secret.nonce().toBase64(QByteArray::Base64Encoding));
QString encodedSecret = QString::fromUtf8(m_secret.cryptText().toBase64(QByteArray::Base64Encoding));
settings.setValue("secret", encodedSecret);
settings.setValue("nonce", encodedNonce);
settings.setValue("timeStep", m_timeStep);
settings.setValue("pinLength", m_tokenLength);
settings.setValue("epoch", m_epoch.toUTC().toString(Qt::ISODateWithMs));
settings.setValue("hash", QVariant::fromValue<Account::Hash>(m_hash).toString());
settings.setValue(QStringLiteral("secret"), encodedSecret);
settings.setValue(QStringLiteral("nonce"), encodedNonce);
settings.setValue(QStringLiteral("timeStep"), m_timeStep);
settings.setValue(QStringLiteral("pinLength"), m_tokenLength);
settings.setValue(QStringLiteral("epoch"), m_epoch.toUTC().toString(Qt::ISODateWithMs));
settings.setValue(QStringLiteral("hash"), QVariant::fromValue<Account::Hash>(m_hash).toString());
settings.endGroup();
// Try to guarantee that data will have been written before claiming the account was actually saved
@ -248,7 +244,7 @@ namespace accounts
}
bool ok = false;
m_settings([this, derived, &ok](QSettings &settings) -> void
m_settings([derived, &ok](QSettings &settings) -> void
{
if (!settings.isWritable()) {
qCWarning(logger) << "Unable to save account secret key parameters: storage not writable";
@ -258,12 +254,12 @@ namespace accounts
const secrets::KeyDerivationParameters params = derived->params();
QString encodedSalt = QString::fromUtf8(derived->salt().toBase64(QByteArray::Base64Encoding));
settings.beginGroup("master-key");
settings.setValue("salt", encodedSalt);
settings.setValue("cpu", params.cpuCost());
settings.setValue("memory", (quint64) params.memoryCost());
settings.setValue("algorithm", params.algorithm());
settings.setValue("length", params.keyLength());
settings.beginGroup(QStringLiteral("master-key"));
settings.setValue(QStringLiteral("salt"), encodedSalt);
settings.setValue(QStringLiteral("cpu"), params.cpuCost());
settings.setValue(QStringLiteral("memory"), (quint64) params.memoryCost());
settings.setValue(QStringLiteral("algorithm"), params.algorithm());
settings.setValue(QStringLiteral("length"), params.keyLength());
settings.endGroup();
ok = true;
});
@ -308,32 +304,32 @@ namespace accounts
}
QStringList groups = settings.childGroups();
if (!groups.contains(QLatin1String("master-key"))) {
if (!groups.contains(QStringLiteral("master-key"))) {
qCInfo(logger) << "No key derivation parameters found: requesting 'new' password for accounts";
ok = m_secret->requestNewPassword();
return;
}
settings.beginGroup("master-key");
settings.beginGroup(QStringLiteral("master-key"));
QByteArray salt;
quint64 cpuCost = 0ULL;
quint64 keyLength = 0ULL;
size_t memoryCost = 0ULL;
int algorithm = settings.value("algorithm").toInt(&ok);
int algorithm = settings.value(QStringLiteral("algorithm")).toInt(&ok);
if (ok) {
ok = false;
keyLength = settings.value("length").toULongLong(&ok);
keyLength = settings.value(QStringLiteral("length")).toULongLong(&ok);
}
if (ok) {
ok = false;
cpuCost = settings.value("cpu").toULongLong(&ok);
cpuCost = settings.value(QStringLiteral("cpu")).toULongLong(&ok);
}
if (ok) {
ok = false;
memoryCost = settings.value("memory").toULongLong(&ok);
memoryCost = settings.value(QStringLiteral("memory")).toULongLong(&ok);
}
if (ok) {
QByteArray encodedSalt = settings.value("salt").toString().toUtf8();
QByteArray encodedSalt = settings.value(QStringLiteral("salt")).toString().toUtf8();
salt = QByteArray::fromBase64(encodedSalt, QByteArray::Base64Encoding);
ok = secrets::SecureMasterKey::validate(salt);
}
@ -384,7 +380,7 @@ namespace accounts
settings.beginGroup(group);
const QString accountName = settings.value("account").toString();
const QString accountName = settings.value(QStringLiteral("account")).toString();
if (!checkName(accountName)) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -393,7 +389,7 @@ namespace accounts
continue;
}
const QString issuer = settings.value("issuer", QString()).toString();
const QString issuer = settings.value(QStringLiteral("issuer"), QString()).toString();
if (!checkIssuer(issuer)) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -402,8 +398,8 @@ namespace accounts
continue;
}
const QString type = settings.value("type").toString();
if (type != QLatin1String("hotp") && type != QLatin1String("totp")) {
const QString type = settings.value(QStringLiteral("type")).toString();
if (type != QStringLiteral("hotp") && type != QStringLiteral("totp")) {
qCWarning(logger)
<< "Skipping invalid account:" << id
<< "Invalid account type";
@ -413,7 +409,7 @@ namespace accounts
}
bool ok = false;
const int tokenLength = settings.value("pinLength").toInt(&ok);
const int tokenLength = settings.value(QStringLiteral("pinLength")).toInt(&ok);
if (!ok || !checkTokenLength(tokenLength)) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -423,8 +419,8 @@ namespace accounts
continue;
}
const QByteArray encodedNonce = settings.value("nonce").toString().toUtf8();
const QByteArray encodedSecret = settings.value("secret").toString().toUtf8();
const QByteArray encodedNonce = settings.value(QStringLiteral("nonce")).toString().toUtf8();
const QByteArray encodedSecret = settings.value(QStringLiteral("secret")).toString().toUtf8();
const QByteArray nonce = QByteArray::fromBase64(encodedNonce, QByteArray::Base64Encoding);
const QByteArray secret = QByteArray::fromBase64(encodedSecret, QByteArray::Base64Encoding);
@ -448,9 +444,9 @@ namespace accounts
continue;
}
if (type == QLatin1String("totp")) {
if (type == QStringLiteral("totp")) {
ok = false;
const uint timeStep = settings.value("timeStep").toUInt(&ok);
const uint timeStep = settings.value(QStringLiteral("timeStep")).toUInt(&ok);
if (!ok || !checkTimeStep(timeStep)) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -460,7 +456,8 @@ namespace accounts
continue;
}
const QDateTime epoch = settings.value("epoch", QDateTime::fromMSecsSinceEpoch(0)).toDateTime();
const QDateTime epoch = settings.value(QStringLiteral("epoch"), QDateTime::fromMSecsSinceEpoch(0))
.toDateTime();
if (!checkEpoch(epoch, m_clock)) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -471,7 +468,10 @@ namespace accounts
}
ok = false;
const QByteArray hashName = settings.value("hash", hashDefault).toByteArray();
const auto hashEnum = QMetaEnum::fromType<accounts::Account::Hash>();
const auto hashDefault = QVariant::fromValue<accounts::Account::Hash>(accounts::Account::Sha1);
const QByteArray hashName = settings.value(QStringLiteral("hash"), hashDefault).toByteArray();
int hash = hashEnum.keyToValue(hashName.constData(), &ok);
if (!ok) {
qCWarning(logger)
@ -487,9 +487,9 @@ namespace accounts
timeStep, epoch, (Account::Hash) hash);
}
if (type == QLatin1String("hotp")) {
if (type == QStringLiteral("hotp")) {
ok = false;
const quint64 counter = settings.value("counter").toULongLong(&ok);
const quint64 counter = settings.value(QStringLiteral("counter")).toULongLong(&ok);
if (!ok) {
qCWarning(logger)
<< "Skipping invalid account:" << id
@ -499,7 +499,7 @@ namespace accounts
continue;
}
const QVariant offsetVariant = settings.value("offset");
const QVariant offsetVariant = settings.value(QStringLiteral("offset"));
ok = offsetVariant.isNull();
std::optional<uint> offset = ok ? std::nullopt : std::optional<uint>(offsetVariant.toUInt(&ok));
@ -512,8 +512,9 @@ namespace accounts
continue;
}
const QString checksum = settings.value("checksum", falseVariantValue).toString();
if (checksum != trueVariantValue && checksum != falseVariantValue) {
const auto checkSumOff = QStringLiteral("false");
const auto checksum = settings.value(QStringLiteral("checksum"), checkSumOff).toString();
if (checksum != QStringLiteral("true") && checksum != checkSumOff) {
qCWarning(logger)
<< "Skipping invalid account:" << id
<< "Invalid checksum";
@ -524,7 +525,8 @@ namespace accounts
qCInfo(logger) << "Found valid HOTP account:" << id;
Q_EMIT foundHotp(id, accountName, issuer, secret, nonce, tokenLength,
counter, offset.has_value(), offset ? *offset : 0U, checksum == trueVariantValue);
counter, offset.has_value(), offset ? *offset : 0U,
checksum == QStringLiteral("true"));
}
settings.endGroup();
@ -555,7 +557,7 @@ namespace accounts
ComputeTotp::ComputeTotp(const AccountSecret *secret,
const secrets::EncryptedSecret &tokenSecret, uint tokenLength,
const QDateTime &epoch, uint timeStep, const Account::Hash &hash,
const QDateTime &epoch, uint timeStep, const Account::Hash hash,
const std::function<qint64(void)> &clock) :
AccountJob(), m_secret(secret), m_tokenSecret(tokenSecret), m_tokenLength(tokenLength),
m_epoch(epoch), m_timeStep(timeStep), m_hash(hash), m_clock(clock)
@ -649,7 +651,7 @@ namespace accounts
ComputeHotp::ComputeHotp(const AccountSecret *secret,
const secrets::EncryptedSecret &tokenSecret, uint tokenLength,
quint64 counter, const std::optional<uint> &offset, bool checksum) :
quint64 counter, const std::optional<uint> offset, bool checksum) :
AccountJob(), m_secret(secret), m_tokenSecret(tokenSecret), m_tokenLength(tokenLength),
m_counter(counter), m_offset(offset), m_checksum(checksum)
{

28
src/account/actions_p.h
View File

@ -23,7 +23,7 @@
namespace accounts
{
class AccountJob: public QObject
class AccountJob: public QObject // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
@ -35,7 +35,7 @@ namespace accounts
void finished(void);
};
class Null: public AccountJob
class Null: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
@ -43,7 +43,7 @@ namespace accounts
void run(void) override;
};
class RequestAccountPassword: public AccountJob
class RequestAccountPassword: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
@ -63,7 +63,7 @@ namespace accounts
bool m_succeeded;
};
class LoadAccounts: public AccountJob
class LoadAccounts: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
@ -84,7 +84,7 @@ namespace accounts
const std::function<qint64(void)> m_clock;
};
class DeleteAccounts: public AccountJob
class DeleteAccounts: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
@ -97,14 +97,14 @@ namespace accounts
const QSet<QUuid> m_ids;
};
class SaveHotp: public AccountJob
class SaveHotp: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
explicit SaveHotp(const SettingsProvider &settings,
const QUuid &id, const QString &accountName, const QString &issuer,
const QUuid id, const QString &accountName, const QString &issuer,
const secrets::EncryptedSecret &secret, uint tokenLength,
quint64 counter, const std::optional<uint> &offset, bool checksum);
quint64 counter, const std::optional<uint> offset, bool checksum);
void run(void) override;
Q_SIGNALS:
void invalid(void);
@ -123,12 +123,12 @@ namespace accounts
const bool m_checksum;
};
class SaveTotp: public AccountJob
class SaveTotp: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
explicit SaveTotp(const SettingsProvider &settings,
const QUuid &id, const QString &accountName, const QString &issuer,
const QUuid id, const QString &accountName, const QString &issuer,
const secrets::EncryptedSecret &secret, uint tokenLength,
uint timeStep, const QDateTime &epoch, Account::Hash hash,
const std::function<qint64(void)> &clock = &QDateTime::currentMSecsSinceEpoch);
@ -151,13 +151,13 @@ namespace accounts
const std::function<qint64(void)> m_clock;
};
class ComputeTotp: public AccountJob
class ComputeTotp: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
explicit ComputeTotp(const AccountSecret *secret,
const secrets::EncryptedSecret &tokenSecret, uint tokenLength,
const QDateTime &epoch, uint timeStep, const Account::Hash &hash,
const QDateTime &epoch, uint timeStep, const Account::Hash hash,
const std::function<qint64(void)> &clock = &QDateTime::currentMSecsSinceEpoch);
void run(void) override;
Q_SIGNALS:
@ -172,13 +172,13 @@ namespace accounts
const std::function<qint64(void)> m_clock;
};
class ComputeHotp: public AccountJob
class ComputeHotp: public AccountJob // clazy:exclude=ctor-missing-parent-argument
{
Q_OBJECT
public:
explicit ComputeHotp(const AccountSecret *secret,
const secrets::EncryptedSecret &tokenSecret, uint tokenLength,
quint64 counter, const std::optional<uint> &offset, bool checksum);
quint64 counter, const std::optional<uint> offset, bool checksum);
void run(void) override;
Q_SIGNALS:
void otp(const QString otp, const QString nextOtp, const quint64 nextCounter);;