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

Add new database wizard and redesign master key widget #1952

Merged
merged 2 commits into from Sep 25, 2018

Conversation

Projects
None yet
7 participants
@phoerious
Copy link
Member

phoerious commented May 13, 2018

Description

This is a WIP PR for adding a user-friendly "New Database" wizard, redesigning the master key change widget, and performing other heavy refactoring in order to make things more streamlined, consistent and reusable.

The PR also fixes several old UX issues, e.g. that canceling database creation still saves a new database without contents and key. It als makes KDBX 4.0 + Argon2-KDF the default setting.

TODO:

  • Implement wizard
  • Create proper wizard image
  • Unify wizard page sizes
  • Properly load KDBX format version from database and save decryption time preference to custom data
  • Redesign master key widget to look nicer and allow keeping individual components unchanged and generally be more useful
  • Merge master key change and general database settings
  • Do not update database encryption settings on database settings save if nothing changed
  • Update / fix tests, write new ones
  • Redesign Key and ChallengeResponseKey classes to use QSharedPointers to unique instances and not allow copying
  • Fuck up heavily improve and redesign other internal APIs around the mentioned changes

Motivation and context

The database setup procedure is very basic and not very friendly towards new and non-technical users. KDF settings etc. are not discoverable, so users will only use the defaults. Also many widgets and APIs are not reusable, which makes it hard to use them in other contexts, such as a wizard.

Resolves #1669, resolves #124

How has this been tested?

n/a

Screenshots (if appropriate):

screen01
screen02
screen03
screen04
screen05
screen06
screen07
screen08
screen09

Types of changes

  • New feature (breaking (!!!) change which adds functionality)

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]
@hifi

This comment has been minimized.

Copy link
Member

hifi commented May 14, 2018

The master key part may need more explanation what to select and why. A new user might think one needs to use all of them. Maybe have the key and challenge response behind an Advanced button?

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented May 14, 2018

I'm planning on redesigning it completely.

@Generator

This comment has been minimized.

Copy link

Generator commented May 19, 2018

This fixes #1669

@TheZ3ro

This comment has been minimized.

Copy link
Member

TheZ3ro commented May 22, 2018

Just 2 notes:

  • the icon in the dialog isn't horizontally aligned
  • set a proper window title instead of "keepassxc"

Anyway, great work

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented May 23, 2018

I know. The icon is just a placeholder until I have something else. ;-)

@phoerious phoerious force-pushed the feature/new-db-wizard branch from 30bc621 to 577600c May 23, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 25, 2018

Need to add the password generator widget to the master key dialog. Maybe even a "Generate..." button which opens the password generator as a modal dialog.

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented May 25, 2018

I've done all that (and much more) already. I will update the screenshots once I've put everything together for you guys to review.

@phoerious phoerious changed the title Add new database wizard Add new database wizard and redesign master key widget May 26, 2018

@phoerious phoerious force-pushed the feature/new-db-wizard branch from ed2b9f0 to 55c9e51 May 26, 2018

@phoerious phoerious force-pushed the feature/new-db-wizard branch from 55c9e51 to a4b1d2f Jun 8, 2018

@droidmonkey droidmonkey requested review from droidmonkey, louib, weslly and TheZ3ro Jul 9, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jul 14, 2018

@phoerious is this ready?

@TheZ3ro

This comment has been minimized.

Copy link
Member

TheZ3ro commented Jul 16, 2018

I don't think so, tests are failing and 4 files are conflicting now

@phoerious phoerious force-pushed the feature/new-db-wizard branch from d26f59e to 9a6c4f6 Jul 24, 2018

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Jul 29, 2018

No, I still need to adjust the tests. I also got feedback on it at the UX Forum, so I want to implement that as well.

@droidmonkey droidmonkey referenced this pull request Aug 24, 2018

Open

UI Redesign #1705

@phoerious phoerious force-pushed the feature/new-db-wizard branch 4 times, most recently from 525d297 to fa0c217 Sep 22, 2018

@phoerious phoerious force-pushed the feature/new-db-wizard branch 3 times, most recently from 4b5b518 to b60186d Sep 23, 2018

@droidmonkey
Copy link
Member

droidmonkey left a comment

Fantastic work!

Show resolved Hide resolved src/core/Config.cpp
m_csvImportWizard = new CsvImportWizard();
m_csvImportWizard->setObjectName("csvImportWizard");
m_databaseSettingsWidget = new DatabaseSettingsWidget();
m_databaseSettingsWidget = new DatabaseSettingsDialog();

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

There is a name mismatch

void DatabaseSettingsDialog::save()
{
if (!m_generalWidget->save())
return;

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

Simple if statements in this whole file need braces

{
int pageIndex = m_ui->stackedWidget->currentIndex();

bool enabled = (pageIndex == 0 && m_generalWidget->hasAdvancedMode());

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

I'd prefer not to use magic numbers for GUI page indexes. Can we shove these into const ints?

Show resolved Hide resolved src/gui/dbsettings/DatabaseSettingsWidgetEncryption.cpp
public:
explicit KeyFileEditWidget(QWidget* parent = nullptr);
Q_DISABLE_COPY(KeyFileEditWidget);
~KeyFileEditWidget();

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

override

Show resolved Hide resolved src/gui/masterkey/KeyComponentWidget.cpp
setWindowTitle(tr("Create a new KeePassXC database..."));

// TODO: change image
setPixmap(QWizard::BackgroundPixmap, filePath()->applicationIcon().pixmap(512, 512));

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

Don't forget to do this 😉

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 24, 2018

Author Member

Never!!!111eleven

Show resolved Hide resolved src/gui/wizard/NewDatabaseWizardPage.h Outdated
setPageWidget(new DatabaseSettingWidgetMetaData());

setTitle(tr("General Database Information"));
setSubTitle(tr("Please fill in the name, and optional description "

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

... display name, optional description, and ...

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 24, 2018

Author Member

I think that was supposed to be an "an". But I just realised that default username is obsolete. It's not shown here anymore.

@droidmonkey
Copy link
Member

droidmonkey left a comment

One additional change

{
setComponentName(tr("YubiKey Challenge-Response"));
setComponentDescription(tr("<p>If you possess a <a href=\"https://www.yubico.com/\">YubiKey</a>, you can add it "
"for additional security.</p><p>The YubiKey needs to be programmed with an "

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 24, 2018

Member

Recommended wording: "If you own a YubiKey, you can use it for additional security. The YubiKey requires one of its slots to be programmed as HMAC-SHA1 Challenge-Response."

@phoerious phoerious force-pushed the feature/new-db-wizard branch 2 times, most recently from 6dd0e88 to 427884b Sep 24, 2018

@phoerious phoerious force-pushed the feature/new-db-wizard branch 2 times, most recently from 96c084c to 5106d6e Sep 24, 2018

@weslly
Copy link
Member

weslly left a comment

Works and looks great to me, but the master password input always show in clear text by default. Is this intended?

@phoerious phoerious force-pushed the feature/new-db-wizard branch from 5106d6e to 414f690 Sep 24, 2018

@phoerious phoerious added EPIC and removed PR: Needs Work labels Sep 24, 2018

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Sep 24, 2018

Yes, it's actually intended, because at that time you need to put some thought into a strong yet memorable master password and it's very unlikely that somebody is should surfing (in which case you can hide it). The password generator inherits whatever state the PasswordEdit field is in (here it really doesn't make any sense to hide it).

@phoerious phoerious force-pushed the feature/new-db-wizard branch from 414f690 to d53da87 Sep 24, 2018

phoerious added some commits May 13, 2018

Add a new database settings wizard
This patch implements a new database wizard to guide users through the process
of setting up a new database and choosing sane encryption settings.

It also reimplements the master key settings to be more
user-friendly. Users can now add, change, or remove individual composite
key components instead of having to set all components at once. This
avoids confusion about a password being reset if the user only wants to
add a key file.

With these changes comes a major refactor of how database composite keys and key
components are handled. Copying of keys is prohibited and each key
exists only once in memory and is referenced via shared pointers. GUI
components for changing individual keys are encapsulated into separate
classes to be more reusable. The password edit and generator widgets
have also been refactored to be more reusable.

@phoerious phoerious force-pushed the feature/new-db-wizard branch from d53da87 to 79be20b Sep 24, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 25, 2018

I approve merging this PR (final 👍 )

@phoerious phoerious merged commit 5295a68 into develop Sep 25, 2018

2 of 3 checks passed

CodeFactor 1 issue fixed. 2 issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@phoerious phoerious deleted the feature/new-db-wizard branch Sep 25, 2018

phoerious added a commit that referenced this pull request Oct 3, 2018

Fix TouchID compiling on macOS (#2332)
Pull request #1952 introduced a compilation error on macOS due to missing header includes.

vi-n added a commit to vi-n/keepassxc that referenced this pull request Oct 3, 2018

Fix TouchID compiling on macOS (keepassxreboot#2332)
Pull request keepassxreboot#1952 introduced a compilation error on macOS due to missing header includes.

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]
@sts10

This comment has been minimized.

Copy link
Contributor

sts10 commented Mar 20, 2019

This is really great! I did have one potentially annoying question... feel free to dismiss for a reason I'm missing!

I feel like it's a bit deceptive to have the number of transform rounds change from a 1 second benchmark to just 1 round when users peak into the Advanced section. Shouldn't it keep the default of a 1 second benchmark given in the simple setup? I'm thinking about users who are curious about the Advanced settings, but don't know to hit the "Benchmark" button...

That said, I'm not sure what a good fix to this is, other than running the 1 second benchmark right when the user clicks the "Advanced" button, which seems not great as the user might think something wrong?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 20, 2019

@sts10 please file a new issue for this, that is a bug

@sts10

This comment has been minimized.

Copy link
Contributor

sts10 commented Mar 20, 2019

Filed: #2806

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.