Refactor browser settings and TOTP by droidmonkey · Pull Request #2284 · keepassxreboot/keepassxc

droidmonkey · 2018-09-11T22:54:50Z

Description

This is a pretty hefty refactor, but well needed (especially TOTP). This aligns BrowserSettings to be similar in design and function to Config and other settings classes. It is now dynamically initialized and accessed using an inline function. Some other static methods/vars were made dynamic due to them being private to their class and only initialized once.

TOTP was completely overhauled, every single function was changed and cleaned up. It was also placed into a namespace instead of a class since all its members are static (desired convention). The previous implementation was extremely brittle and required state to be stored across several different classes. Users of the previous TOTP class were required to know how it worked and when to supply proper variables (BAD!!). The new implementation is completely plug-and-play, no manual required!

Closes #2132

Motivation and context

Clean code == happy maintainers

How has this been tested?

Manually and with written tests

Screenshots (if appropriate):

No GUI layouts were harmed in this refactor. The SetupTotpDialog.ui only changes to using SettingsGroups to control radio buttons.

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ I have added tests to cover my changes.

varjolintu

Everything worked great. Good job!

phoerious · 2018-09-13T08:08:51Z

src/core/Entry.cpp

-        }
-        m_attributes->set("TOTP Settings", data);
+        m_attributes->set("TOTP Seed", m_data.totpSettings->key, true);
+        m_attributes->set("TOTP Settings", text);


I wonder if we could make this translatable somehow. But I think you should at least use a static symbol for this instead of repeating the raw string everywhere.

I would not translate an attribute key. That would also break compat with the KeePass2 plugins.

phoerious · 2018-09-13T08:10:59Z

src/gui/SetupTotpDialog.h

-private Q_SLOTS:
-    void toggleDefault(bool status);
-    void toggleSteam(bool status);
+    explicit SetupTotpDialog(QWidget *parent = nullptr, Entry* entry = nullptr);


QWidget*

This class should also be called TotpSetupDialog.

I agree with the name, I did consider that change

phoerious · 2018-09-13T08:13:27Z

src/gui/TotpDialog.cpp

+        m_ui->progressBar->setValue(100 - m_counter);
        m_ui->progressBar->update();
-        uCounter++;
+        m_counter++;


++m_counter

phoerious · 2018-09-13T08:17:14Z

src/totp/totp.h

+static const QString STEAM_SHORTNAME = "S";
+
+QSharedPointer<Totp::Settings> parseSettings(const QString& rawSettings, const QString& key=QString());
+QSharedPointer<Totp::Settings> createSettings(const QString& key, const uint digits, const uint step,


Spaces around operators and use {} instead of QString().

droidmonkey · 2018-09-14T22:21:56Z

All requested changes were made @phoerious

weslly

For some reason all my TOTP codes stopped working, but the tests still pass. I'll try to debug it

droidmonkey · 2018-09-15T01:28:11Z

Ok good test, I agree the tests passed and was worried about this exact scenario. Check your system time.

weslly · 2018-09-15T12:02:00Z

src/totp/totp.cpp

+
+    quint64 current;
+    if (time == 0) {
+        current = qToBigEndian(QDateTime::currentDateTime().toTime_t() / step);


Found the bug!

toTime_t() returns a 32bit uint, but it worked with the old code because it was stored in a quint64 variable before being passed to qToBigEndian.

toTime_t() is also obsolete since qt 5.8 and we should eventually replace it with toSecsSinceEpoch(), which already returns a qint64.

I'll use the new function, seems the old one is way overdue for deletion. The tests don't pick this up since it requires "real time" to work.

toSecsSinceEpoch() was introduced in 5.8 so we would need to increase the minimum Qt version requirement at https://github.com/keepassxreboot/keepassxc/blob/develop/INSTALL.md too

* Convert BrowserSettings into instanced class * Moved HostInstaller init into class constructor

* CSV Import and Entry Model

* Eliminate TOTP logic from GUI elements * Consolidate TOTP functionality under the Totp namespace * Eliminate guessing about state and encoders * Increased test cases * Add entry view column for TOTP [#2132] * General code cleanup, reduction of unnecessary steps, separation of concerns * Rename SetupTotpDialog to TotpSetupDialog for consistency

droidmonkey · 2018-09-15T16:25:19Z

Fixed the TOTP fail and cleaned up the commit list, this is ready for merge once approvals are granted.

- New Database Wizard [#1952] - Advanced Search [#1797] - Automatic update checker [#2648] - KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739] - Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439] - Remove KeePassHttp support [#1752] - CLI: output info to stderr for easier scripting [#2558] - CLI: Add --quiet option [#2507] - CLI: Add create command [#2540] - CLI: Add recursive listing of entries [#2345] - CLI: Fix stdin/stdout encoding on Windows [#2425] - SSH Agent: Support OpenSSH for Windows [#1994] - macOS: TouchID Quick Unlock [#1851] - macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583] - Linux: Prevent Klipper from storing secrets in clipboard [#1969] - Linux: Use polling based file watching for NFS [#2171] - Linux: Enable use of browser plugin in Snap build [#2802] - TOTP QR Code Generator [#1167] - High-DPI Scaling for 4k screens [#2404] - Make keyboard shortcuts more consistent [#2431] - Warn user if deleting referenced entries [#1744] - Allow toolbar to be hidden and repositioned [#1819, #2357] - Increase max allowed database timeout to 12 hours [#2173] - Password generator uses existing password length by default [#2318] - Improve alert message box button labels [#2376] - Show message when a database merge makes no changes [#2551] - Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790] - Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]

droidmonkey added the high priority 🚨 label Sep 11, 2018

droidmonkey added this to the v2.4.0 milestone Sep 11, 2018

droidmonkey requested review from phoerious and varjolintu September 11, 2018 22:54

droidmonkey force-pushed the refactor/static-init branch 2 times, most recently from 5399b23 to 46fe646 Compare September 11, 2018 23:51

varjolintu approved these changes Sep 12, 2018

View reviewed changes

droidmonkey mentioned this pull request Sep 12, 2018

Debug build fails with clang++ #2290

Closed

phoerious requested changes Sep 13, 2018

View reviewed changes

droidmonkey force-pushed the refactor/static-init branch from c7185a3 to 7bc74bb Compare September 14, 2018 22:21

weslly reviewed Sep 14, 2018

View reviewed changes

weslly suggested changes Sep 15, 2018

View reviewed changes

droidmonkey added 3 commits September 15, 2018 12:10

Reduce use of static vars in browser plugin

53a17c2

* Convert BrowserSettings into instanced class * Moved HostInstaller init into class constructor

Convert private static vars into member vars

b74fb3e

* CSV Import and Entry Model

droidmonkey force-pushed the refactor/static-init branch from 7bc74bb to 96d4ce2 Compare September 15, 2018 16:23

droidmonkey force-pushed the refactor/static-init branch from 96d4ce2 to f4afda6 Compare September 15, 2018 16:56

weslly approved these changes Sep 15, 2018

View reviewed changes

droidmonkey force-pushed the refactor/static-init branch from f4afda6 to 1d525c8 Compare September 15, 2018 19:31

Fix TOTP epoch time; add null checks for settings

823a916

droidmonkey force-pushed the refactor/static-init branch from 1d525c8 to 823a916 Compare September 15, 2018 21:09

phoerious approved these changes Sep 16, 2018

View reviewed changes

droidmonkey merged commit c0aa1ef into develop Sep 16, 2018

droidmonkey deleted the refactor/static-init branch September 16, 2018 12:29

droidmonkey mentioned this pull request Nov 7, 2018

Feature Request: Custom Columns #2461

Closed

childnode mentioned this pull request Oct 8, 2019

TOTP: OTP settings not updated if in invalid format [DATA LOSS] #3142

Closed

Uh oh!

Conversation

droidmonkey commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

varjolintu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Sep 14, 2018

Uh oh!

weslly left a comment

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weslly Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Sep 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

droidmonkey commented Sep 11, 2018 •

edited

Loading

droidmonkey commented Sep 15, 2018 •

edited

Loading

weslly Sep 15, 2018 •

edited

Loading

droidmonkey Sep 15, 2018 •

edited

Loading