Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor browser settings and TOTP #2284

Merged
merged 4 commits into from Sep 16, 2018

Conversation

Projects
None yet
4 participants
@droidmonkey
Copy link
Member

droidmonkey commented Sep 11, 2018

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.

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

@droidmonkey droidmonkey requested review from phoerious and varjolintu Sep 11, 2018

@droidmonkey droidmonkey force-pushed the refactor/static-init branch 2 times, most recently from 5399b23 to 46fe646 Sep 11, 2018

@varjolintu
Copy link
Member

varjolintu left a comment

Everything worked great. Good job!

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

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 13, 2018

Member

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.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 13, 2018

Author Member

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

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

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 13, 2018

Member

QWidget*

This class should also be called TotpSetupDialog.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 13, 2018

Author Member

I agree with the name, I did consider that change

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

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 13, 2018

Member

++m_counter

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,

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 13, 2018

Member

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

@droidmonkey droidmonkey force-pushed the refactor/static-init branch from c7185a3 to 7bc74bb Sep 14, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member Author

droidmonkey commented Sep 14, 2018

All requested changes were made @phoerious

@weslly
Copy link
Member

weslly left a comment

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

@droidmonkey

This comment has been minimized.

Copy link
Member Author

droidmonkey commented Sep 15, 2018

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


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

This comment has been minimized.

Copy link
@weslly

weslly Sep 15, 2018

Member

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.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 15, 2018

Author Member

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.

This comment has been minimized.

Copy link
@weslly

weslly Sep 15, 2018

Member

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

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 15, 2018

Author Member

ugh...

droidmonkey added some commits Sep 5, 2018

Reduce use of static vars in browser plugin
* Convert BrowserSettings into instanced class
* Moved HostInstaller init into class constructor
Convert private static vars into member vars
* CSV Import and Entry Model
Complete refactor of TOTP integration
* 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 droidmonkey force-pushed the refactor/static-init branch from 7bc74bb to 96d4ce2 Sep 15, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member Author

droidmonkey commented Sep 15, 2018

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

@droidmonkey droidmonkey force-pushed the refactor/static-init branch from 96d4ce2 to f4afda6 Sep 15, 2018

@weslly

weslly approved these changes Sep 15, 2018

@droidmonkey droidmonkey force-pushed the refactor/static-init branch from f4afda6 to 1d525c8 Sep 15, 2018

@droidmonkey droidmonkey force-pushed the refactor/static-init branch from 1d525c8 to 823a916 Sep 15, 2018

@droidmonkey droidmonkey merged commit c0aa1ef into develop Sep 16, 2018

2 checks passed

CodeFactor 1 issue fixed.
Details
Debug [Clang] (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the refactor/static-init branch Sep 16, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.