From 9055fb307d05ad76775dcb06891c4ac9c4bf5f1c Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 03:26:16 -0700 Subject: [PATCH 01/10] First pass at adding age penalty to health check --- src/core/Entry.cpp | 27 +++++++++++++++++++++++++++ src/core/Entry.h | 1 + src/core/PasswordHealth.cpp | 9 +++++++++ 3 files changed, 37 insertions(+) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index ad7ecce2b..6ebc86421 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -231,6 +231,33 @@ const QSharedPointer Entry::passwordHealth() const return m_data.passwordHealth; } +int Entry::getPasswordAgeInDays() const +{ + QListIterator i(m_history); + i.toBack(); + Entry* compare = nullptr; + Entry* curr = nullptr; + + while (i.hasPrevious()) { + curr = i.previous(); + if (!compare) { + compare = curr; + continue; + } + if (*curr->attributes() != *compare->attributes()) { + if (curr->password() != compare->password()) { + // Found most recent password change; break out. + break; + } + } + } + if (!curr) { + // If no change in history, password is from creation time + return this->timeInfo().creationTime().daysTo(QDateTime::currentDateTime()); + } + return curr->timeInfo().lastModificationTime().daysTo(QDateTime::currentDateTime()); +} + bool Entry::excludeFromReports() const { return m_data.excludeFromReports diff --git a/src/core/Entry.h b/src/core/Entry.h index 7fc69c8fb..244dd63b4 100644 --- a/src/core/Entry.h +++ b/src/core/Entry.h @@ -116,6 +116,7 @@ public: QString path() const; const QSharedPointer passwordHealth(); const QSharedPointer passwordHealth() const; + int getPasswordAgeInDays() const; bool excludeFromReports() const; void setExcludeFromReports(bool state); diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index 3225affb3..16c602dbd 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -198,6 +198,15 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const } } + // Fourth, reduce score by 5 for each year beyond one year old. + int age = entry->getPasswordAgeInDays(); + int ageInYears = age / 365; + if (ageInYears > 1) { + constexpr auto penalty = 5; + health->adjustScore(-penalty * ageInYears); + health->addScoreReason(QObject::tr("Password is %1 year(s) old", "", ageInYears).arg(ageInYears)); + } + // Return the result return health; } From 1121488438b9a2964b09f16a5f9fb2d85316008c Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 04:16:58 -0700 Subject: [PATCH 02/10] Removed penalty for old age --- src/core/PasswordHealth.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index 16c602dbd..1d23c8840 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -202,8 +202,6 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const int age = entry->getPasswordAgeInDays(); int ageInYears = age / 365; if (ageInYears > 1) { - constexpr auto penalty = 5; - health->adjustScore(-penalty * ageInYears); health->addScoreReason(QObject::tr("Password is %1 year(s) old", "", ageInYears).arg(ageInYears)); } From f90483cc1f8efb80d14c9224504fcf76eee5bc5d Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 05:00:18 -0700 Subject: [PATCH 03/10] Switched to seconds for getPasswordAge for better reuse --- src/core/Entry.cpp | 6 +++--- src/core/Entry.h | 2 +- src/core/PasswordHealth.cpp | 11 +++++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 6ebc86421..3679ee71e 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -231,7 +231,7 @@ const QSharedPointer Entry::passwordHealth() const return m_data.passwordHealth; } -int Entry::getPasswordAgeInDays() const +int Entry::getPasswordAge() const { QListIterator i(m_history); i.toBack(); @@ -253,9 +253,9 @@ int Entry::getPasswordAgeInDays() const } if (!curr) { // If no change in history, password is from creation time - return this->timeInfo().creationTime().daysTo(QDateTime::currentDateTime()); + return this->timeInfo().creationTime().secsTo(QDateTime::currentDateTime()); } - return curr->timeInfo().lastModificationTime().daysTo(QDateTime::currentDateTime()); + return curr->timeInfo().lastModificationTime().secsTo(QDateTime::currentDateTime()); } bool Entry::excludeFromReports() const diff --git a/src/core/Entry.h b/src/core/Entry.h index 244dd63b4..1ac7c5763 100644 --- a/src/core/Entry.h +++ b/src/core/Entry.h @@ -116,7 +116,7 @@ public: QString path() const; const QSharedPointer passwordHealth(); const QSharedPointer passwordHealth() const; - int getPasswordAgeInDays() const; + int getPasswordAge() const; bool excludeFromReports() const; void setExcludeFromReports(bool state); diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index 1d23c8840..4556b1ebb 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -19,6 +19,7 @@ #include "Group.h" #include "PasswordHealth.h" +#include "Tools.h" #include "zxcvbn.h" namespace @@ -199,10 +200,12 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const } // Fourth, reduce score by 5 for each year beyond one year old. - int age = entry->getPasswordAgeInDays(); - int ageInYears = age / 365; - if (ageInYears > 1) { - health->addScoreReason(QObject::tr("Password is %1 year(s) old", "", ageInYears).arg(ageInYears)); + int ageInSeconds = entry->getPasswordAge(); + // (365 days)(24 hours/day)(3600 s/hr) is approximately a year and gets compiled away. + // Unfortunately, Qt doesn't seem to have a utility for this. + if (ageInSeconds / (365 * 24 * 3600) > 1) { + Tools::humanReadableTimeDifference(ageInSeconds); + health->addScoreReason(QObject::tr("Password is %1 old").arg(Tools::humanReadableTimeDifference(ageInSeconds))); } // Return the result From f2951443e1210fe0304f1b87004ae88d0a55740b Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 05:12:39 -0700 Subject: [PATCH 04/10] Fixed outdated comments --- src/core/PasswordHealth.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index 4556b1ebb..8b5998d41 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -199,10 +199,10 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const } } - // Fourth, reduce score by 5 for each year beyond one year old. + // Fourth, add note if password is two or more years old. int ageInSeconds = entry->getPasswordAge(); + // Unfortunately, Qt doesn't seem to have a utility for seconds->year. // (365 days)(24 hours/day)(3600 s/hr) is approximately a year and gets compiled away. - // Unfortunately, Qt doesn't seem to have a utility for this. if (ageInSeconds / (365 * 24 * 3600) > 1) { Tools::humanReadableTimeDifference(ageInSeconds); health->addScoreReason(QObject::tr("Password is %1 old").arg(Tools::humanReadableTimeDifference(ageInSeconds))); From d85f3a2b82dfb2ec3ece2969646bd874cc290fbf Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 05:16:01 -0700 Subject: [PATCH 05/10] Removed one other extraneous line --- src/core/PasswordHealth.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index 8b5998d41..c7d00f761 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -204,7 +204,6 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const // Unfortunately, Qt doesn't seem to have a utility for seconds->year. // (365 days)(24 hours/day)(3600 s/hr) is approximately a year and gets compiled away. if (ageInSeconds / (365 * 24 * 3600) > 1) { - Tools::humanReadableTimeDifference(ageInSeconds); health->addScoreReason(QObject::tr("Password is %1 old").arg(Tools::humanReadableTimeDifference(ageInSeconds))); } From 28f1cd398db52f5a9daff379b21c248bf8c8af10 Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 14:05:38 -0700 Subject: [PATCH 06/10] getPasswordAge unit tests. Tests fail because QDateTime::currentDateTime() is not using the MockClock instance. The time is correct, but as it changes every time it makes the test non-deterministic. --- tests/CMakeLists.txt | 2 +- tests/TestEntry.cpp | 38 ++++++++++++++++++++++++++++++++++++++ tests/TestEntry.h | 1 + 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index db82da163..06fea2107 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -168,7 +168,7 @@ if(WITH_XC_SSHAGENT) endif() add_unit_test(NAME testentry SOURCES TestEntry.cpp - LIBS ${TEST_LIBRARIES}) + LIBS testsupport ${TEST_LIBRARIES}) add_unit_test(NAME testmerge SOURCES TestMerge.cpp LIBS testsupport ${TEST_LIBRARIES}) diff --git a/tests/TestEntry.cpp b/tests/TestEntry.cpp index 07519d94b..ffdf6cb51 100644 --- a/tests/TestEntry.cpp +++ b/tests/TestEntry.cpp @@ -19,6 +19,8 @@ #include #include "TestEntry.h" +#include "mock/MockClock.h" + #include "core/Clock.h" #include "core/Group.h" #include "core/Metadata.h" @@ -47,6 +49,42 @@ void TestEntry::testHistoryItemDeletion() QVERIFY(historyEntry.isNull()); } +void TestEntry::testGetPasswordAge() +{ + MockClock* m_clock = new MockClock(2022, 2, 4, 17, 00, 00); + MockClock::setup(m_clock); + + // Old password updated 100 seconds ago + QPointer historyEntry = new Entry(); + historyEntry->setPassword("oldpassword"); + m_clock->advanceSecond(500); + QScopedPointer entry(new Entry()); + entry->setPassword("newpassword"); + entry->addHistoryItem(historyEntry); + m_clock->advanceSecond(100); + + QCOMPARE(entry->getPasswordAge(), 100); + + QPointer historyEntry2 = new Entry(); + historyEntry2->setPassword("oldpassword"); + m_clock->advanceSecond(500); + QScopedPointer entry2(new Entry()); + entry2->setPassword("oldpassword"); + + // No history, password just created + QCOMPARE(entry2->getPasswordAge(), 0); + m_clock->advanceSecond(100); + // 100 seconds pass since creation + QCOMPARE(entry2->getPasswordAge(), 100); + + entry2->addHistoryItem(historyEntry2); + // History entry shows password is actually + // 500 seconds older than the most recent update + QCOMPARE(entry2->getPasswordAge(), 600); + + MockClock::teardown(); +} + void TestEntry::testCopyDataFrom() { QScopedPointer entry(new Entry()); diff --git a/tests/TestEntry.h b/tests/TestEntry.h index 3bfd8f52d..fa00fd508 100644 --- a/tests/TestEntry.h +++ b/tests/TestEntry.h @@ -29,6 +29,7 @@ class TestEntry : public QObject private slots: void initTestCase(); void testHistoryItemDeletion(); + void testGetPasswordAge(); void testCopyDataFrom(); void testClone(); void testResolveUrl(); From c14725cf9796269fdaab5461719913c35777354d Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 17:09:38 -0700 Subject: [PATCH 07/10] Fixed tests and improved age logic --- src/core/Entry.cpp | 30 ++++++++++++++---------------- tests/TestEntry.cpp | 18 +++++++++++++++++- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 3679ee71e..4d997e4ab 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -235,27 +235,25 @@ int Entry::getPasswordAge() const { QListIterator i(m_history); i.toBack(); - Entry* compare = nullptr; - Entry* curr = nullptr; + const Entry* curr = nullptr; + const Entry* previous = this; while (i.hasPrevious()) { - curr = i.previous(); - if (!compare) { - compare = curr; - continue; - } - if (*curr->attributes() != *compare->attributes()) { - if (curr->password() != compare->password()) { - // Found most recent password change; break out. - break; - } + curr = previous; + previous = i.previous(); + if (previous->password() != curr->password()) { + // Found last change in history + return curr->timeInfo().lastModificationTime().secsTo(Clock::currentDateTime()); } + } - if (!curr) { - // If no change in history, password is from creation time - return this->timeInfo().creationTime().secsTo(QDateTime::currentDateTime()); + if (previous!=this) { + // If no change in history, password is from oldest history entry. + // Not using creation time here because that changes when an entry is cloned + return previous->timeInfo().lastModificationTime().secsTo(Clock::currentDateTime()); } - return curr->timeInfo().lastModificationTime().secsTo(QDateTime::currentDateTime()); + // If no history, creation time is when the password appeared + return this->timeInfo().creationTime().secsTo(Clock::currentDateTime()); } bool Entry::excludeFromReports() const diff --git a/tests/TestEntry.cpp b/tests/TestEntry.cpp index ffdf6cb51..ea44ee6a4 100644 --- a/tests/TestEntry.cpp +++ b/tests/TestEntry.cpp @@ -79,9 +79,25 @@ void TestEntry::testGetPasswordAge() entry2->addHistoryItem(historyEntry2); // History entry shows password is actually - // 500 seconds older than the most recent update + // 500 seconds older than the creation time QCOMPARE(entry2->getPasswordAge(), 600); + // Bury password change in history + entry2->setPassword("newpassword"); + QPointer historyEntry3 = new Entry(); + historyEntry3->setPassword("newpassword"); + entry->addHistoryItem(historyEntry3); + m_clock->advanceSecond(400); + QPointer historyEntry4 = new Entry(); + historyEntry4->setPassword("newpassword"); + entry->addHistoryItem(historyEntry4); + m_clock->advanceSecond(400); + QCOMPARE(entry2->getPasswordAge(), 800); + + // Second test where current password is the latest + entry2->setPassword("newerpassword"); + QCOMPARE(entry2->getPasswordAge(), 0); + MockClock::teardown(); } From 00d2b3f9b0f91b94b9f13c92477527caa3cf8511 Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 17:23:41 -0700 Subject: [PATCH 08/10] Formatting --- src/core/Entry.cpp | 3 +-- tests/TestEntry.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 4d997e4ab..81702fd7a 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -245,9 +245,8 @@ int Entry::getPasswordAge() const // Found last change in history return curr->timeInfo().lastModificationTime().secsTo(Clock::currentDateTime()); } - } - if (previous!=this) { + if (previous != this) { // If no change in history, password is from oldest history entry. // Not using creation time here because that changes when an entry is cloned return previous->timeInfo().lastModificationTime().secsTo(Clock::currentDateTime()); diff --git a/tests/TestEntry.cpp b/tests/TestEntry.cpp index ea44ee6a4..b0c88bcc9 100644 --- a/tests/TestEntry.cpp +++ b/tests/TestEntry.cpp @@ -78,7 +78,7 @@ void TestEntry::testGetPasswordAge() QCOMPARE(entry2->getPasswordAge(), 100); entry2->addHistoryItem(historyEntry2); - // History entry shows password is actually + // History entry shows password is actually // 500 seconds older than the creation time QCOMPARE(entry2->getPasswordAge(), 600); From 4a39b1be544e6d08f0e1576aa17f888b06186d04 Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Tue, 1 Nov 2022 18:29:50 -0700 Subject: [PATCH 09/10] Translations --- share/translations/keepassxc_en.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 2701f7b05..a526851ec 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -7924,6 +7924,10 @@ Kernel: %3 %4 This options is deprecated, use --set-key-file instead. + + Password is %1 old + + QtIOCompressor From 676f64f7b27e93b2bca7979751705a11f2dc2da3 Mon Sep 17 00:00:00 2001 From: mattesony <49170923+mattesony@users.noreply.github.com> Date: Mon, 30 Jan 2023 14:06:20 -0800 Subject: [PATCH 10/10] Rename Entry::getPasswordAge() to Entry::passwordAgeSeconds() --- src/core/Entry.cpp | 2 +- src/core/Entry.h | 2 +- src/core/PasswordHealth.cpp | 2 +- tests/TestEntry.cpp | 30 +++++++++++++++--------------- tests/TestEntry.h | 2 +- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 81702fd7a..48fd6a769 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -231,7 +231,7 @@ const QSharedPointer Entry::passwordHealth() const return m_data.passwordHealth; } -int Entry::getPasswordAge() const +int Entry::passwordAgeSeconds() const { QListIterator i(m_history); i.toBack(); diff --git a/src/core/Entry.h b/src/core/Entry.h index 1ac7c5763..3b3dabd29 100644 --- a/src/core/Entry.h +++ b/src/core/Entry.h @@ -116,7 +116,7 @@ public: QString path() const; const QSharedPointer passwordHealth(); const QSharedPointer passwordHealth() const; - int getPasswordAge() const; + int passwordAgeSeconds() const; bool excludeFromReports() const; void setExcludeFromReports(bool state); diff --git a/src/core/PasswordHealth.cpp b/src/core/PasswordHealth.cpp index c7d00f761..5d085df4b 100644 --- a/src/core/PasswordHealth.cpp +++ b/src/core/PasswordHealth.cpp @@ -200,7 +200,7 @@ QSharedPointer HealthChecker::evaluate(const Entry* entry) const } // Fourth, add note if password is two or more years old. - int ageInSeconds = entry->getPasswordAge(); + int ageInSeconds = entry->passwordAgeSeconds(); // Unfortunately, Qt doesn't seem to have a utility for seconds->year. // (365 days)(24 hours/day)(3600 s/hr) is approximately a year and gets compiled away. if (ageInSeconds / (365 * 24 * 3600) > 1) { diff --git a/tests/TestEntry.cpp b/tests/TestEntry.cpp index b0c88bcc9..70daff4d2 100644 --- a/tests/TestEntry.cpp +++ b/tests/TestEntry.cpp @@ -49,54 +49,54 @@ void TestEntry::testHistoryItemDeletion() QVERIFY(historyEntry.isNull()); } -void TestEntry::testGetPasswordAge() +void TestEntry::testPasswordAgeSeconds() { - MockClock* m_clock = new MockClock(2022, 2, 4, 17, 00, 00); - MockClock::setup(m_clock); + MockClock* mockClock = new MockClock(2022, 2, 4, 17, 00, 00); + MockClock::setup(mockClock); // Old password updated 100 seconds ago QPointer historyEntry = new Entry(); historyEntry->setPassword("oldpassword"); - m_clock->advanceSecond(500); + mockClock->advanceSecond(500); QScopedPointer entry(new Entry()); entry->setPassword("newpassword"); entry->addHistoryItem(historyEntry); - m_clock->advanceSecond(100); + mockClock->advanceSecond(100); - QCOMPARE(entry->getPasswordAge(), 100); + QCOMPARE(entry->passwordAgeSeconds(), 100); QPointer historyEntry2 = new Entry(); historyEntry2->setPassword("oldpassword"); - m_clock->advanceSecond(500); + mockClock->advanceSecond(500); QScopedPointer entry2(new Entry()); entry2->setPassword("oldpassword"); // No history, password just created - QCOMPARE(entry2->getPasswordAge(), 0); - m_clock->advanceSecond(100); + QCOMPARE(entry2->passwordAgeSeconds(), 0); + mockClock->advanceSecond(100); // 100 seconds pass since creation - QCOMPARE(entry2->getPasswordAge(), 100); + QCOMPARE(entry2->passwordAgeSeconds(), 100); entry2->addHistoryItem(historyEntry2); // History entry shows password is actually // 500 seconds older than the creation time - QCOMPARE(entry2->getPasswordAge(), 600); + QCOMPARE(entry2->passwordAgeSeconds(), 600); // Bury password change in history entry2->setPassword("newpassword"); QPointer historyEntry3 = new Entry(); historyEntry3->setPassword("newpassword"); entry->addHistoryItem(historyEntry3); - m_clock->advanceSecond(400); + mockClock->advanceSecond(400); QPointer historyEntry4 = new Entry(); historyEntry4->setPassword("newpassword"); entry->addHistoryItem(historyEntry4); - m_clock->advanceSecond(400); - QCOMPARE(entry2->getPasswordAge(), 800); + mockClock->advanceSecond(400); + QCOMPARE(entry2->passwordAgeSeconds(), 800); // Second test where current password is the latest entry2->setPassword("newerpassword"); - QCOMPARE(entry2->getPasswordAge(), 0); + QCOMPARE(entry2->passwordAgeSeconds(), 0); MockClock::teardown(); } diff --git a/tests/TestEntry.h b/tests/TestEntry.h index fa00fd508..ec0bd5e19 100644 --- a/tests/TestEntry.h +++ b/tests/TestEntry.h @@ -29,7 +29,7 @@ class TestEntry : public QObject private slots: void initTestCase(); void testHistoryItemDeletion(); - void testGetPasswordAge(); + void testPasswordAgeSeconds(); void testCopyDataFrom(); void testClone(); void testResolveUrl();