Add support for multiple autotype sequences for a single entry #1390

TheZ3ro · 2018-01-16T21:11:50Z

Description

This PR:

adds support for multiple autotype sequences for a single entry (based from Fixes #347 - Support multiple auto-type matches per entry keepassx/keepassx#132)
fixes some bug with the effectiveAutoTypeSequence
add tests for effectiveAutoTypeSequence
refactor AutoType functions
improved the AutoType Window Association UI/UX

Motivation and context

Fixes #559

How has this been tested?

Manually and with make tests

Screenshots (if appropriate):

Types of changes

✅ New feature (non-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]
✅ I have added tests to cover my changes.

TheZ3ro · 2018-01-18T23:55:28Z

Ready for review 🏁

phoerious · 2018-01-25T20:08:46Z

src/autotype/AutoType.cpp

+{
+    if (m_executor) {
+        delete m_executor;
+        m_executor = nullptr;


Change m_executor to a QPointer and remove nullptr assignments.

AutoTypeExecutor doesn't inherit from QObject so you can't use QPointer here

Any strong objections against changing that?

Seems that this triggers some error with the linker

I would prefer changing this in a follow up PR

Shouldn't cause linker errors.

phoerious · 2018-01-25T20:13:31Z

src/autotype/AutoType.cpp

+            m_currentGlobalKey = key;
+            m_currentGlobalModifiers = modifiers;
+            return true;
+        } else {


Drop redundant else.

phoerious · 2018-01-25T20:13:43Z

src/autotype/AutoType.cpp

+            return true;
+        } else {
+            return false;
+        }
    } else {


phoerious · 2018-01-25T20:15:35Z

src/autotype/AutoType.cpp

+    }
+
+    QList<QString> sequences = autoTypeSequences(entry);
+    if(sequences.isEmpty()) {


Space after if.

phoerious · 2018-01-25T20:15:58Z

src/autotype/AutoType.cpp

-                entryList << entry;
-                sequenceHash.insert(entry, sequence);
+            const QList<QString> sequences = autoTypeSequences(entry, windowTitle);
+            for (QString sequence : sequences) {


It's a QList<QString>, adding & trigger this compiler error error: binding ‘const QString’ to reference of type ‘QString&’ discards qualifiers

Yeah, well. const QString& it is then. ;-)

phoerious · 2018-01-25T20:44:58Z

src/gui/entry/AutoTypeMatchView.h

+protected:
+    void keyPressEvent(QKeyEvent* event) override;
+
+private Q_SLOTS:


phoerious · 2018-01-25T20:45:08Z

src/gui/entry/EditEntryWidget.cpp

@@ -170,8 +170,7 @@ void EditEntryWidget::setupAutoType()

    m_autoTypeDefaultSequenceGroup->addButton(m_autoTypeUi->inheritSequenceButton);
    m_autoTypeDefaultSequenceGroup->addButton(m_autoTypeUi->customSequenceButton);
-    m_autoTypeWindowSequenceGroup->addButton(m_autoTypeUi->defaultWindowSequenceButton);
-    m_autoTypeWindowSequenceGroup->addButton(m_autoTypeUi->customWindowSequenceButton);
+    //m_autoTypeWindowSequenceGroup->addButton(m_autoTypeUi->customWindowSequenceButton);


phoerious · 2018-01-25T20:46:09Z

tests/TestAutoType.cpp

+    QString sequenceDisabled("{TEST_DISABLED}");
+    QString sequenceOrphan("{TEST_ORPHAN}");
+
+    Database* db = new Database();


QScopedPointer

phoerious · 2018-01-25T20:50:33Z

src/autotype/AutoType.cpp

-        return;
+    Q_ASSERT(m_inAutoType);
+
+    m_inAutoType = false;


This should be a QMutex, which is tried with tryLock().

phoerious · 2018-01-25T20:51:14Z

src/autotype/AutoType.cpp

+        return;
+    }
+
+    m_inAutoType = true;


When changed to a QMutex, use a QMutexLocker here.

weslly · 2018-01-30T21:38:48Z

@TheZ3ro It duplicates the entry item if I click 'Cancel' and call the auto-type dialog again:

For some reason it doesn't happen on some entries.

EDIT: It seems to be happening on entries with another cloned entry in the recycle bin, even though the entry title is different.

TheZ3ro · 2018-01-30T23:12:55Z

@weslly I can't reproduce it on GNU/Linux. Cloned a bunch of entry, deleted some of them, still no duplicate when clicking on "Cancel".
Can you check out if the "Recycle Bin" group has autotype disabled?

weslly · 2018-01-31T01:31:48Z

@TheZ3ro I tried with a new database with only one entry and without the recycle bin group and still got it (this time on the 1st attempt, without canceling first):

TheZ3ro · 2018-01-31T20:25:28Z

@weslly Ok, I've sort it out.

If you have the MatchTitle and MatchURL option enabled, in some cases you will end up with 2 or even 3 repeated sequence since:

First sequence comes from matching entry Title with window-Title
Second sequence comes from matching entry URL with window-Title
Eventually, a third sequence comes from window association

Using a QSet is a viable option, duplicated sequences won't be inserted

weslly · 2018-02-01T12:03:04Z

@TheZ3ro the duplicate sequence bug is fixed now but I noticed the window no longer activates after selecting the entry on macOS (it does work on linux though). Probably raiseLastActiveWindow isn't working correctly.

I'll try to find what's causing this.

weslly · 2018-02-01T21:40:20Z

@TheZ3ro actually the window activates after selecting the auto-type entry, but only if I click it with the mouse. If I just press enter to get the 1st item it closes the window without performing the sequence.

weslly · 2018-02-02T00:35:40Z

src/autotype/AutoTypeSelectDialog.cpp

@@ -56,21 +58,20 @@ AutoTypeSelectDialog::AutoTypeSelectDialog(QWidget* parent)
    QLabel* descriptionLabel = new QLabel(tr("Select entry to Auto-Type:"), this);
    layout->addWidget(descriptionLabel);

-    connect(m_view, SIGNAL(activated(QModelIndex)), SLOT(emitEntryActivated(QModelIndex)));
-    connect(m_view, SIGNAL(clicked(QModelIndex)), SLOT(emitEntryActivated(QModelIndex)));
+    connect(m_view, SIGNAL(activated(QModelIndex)), SLOT(emitMatchActivated(QModelIndex)));


This signal is being completely ignored on macOS. This may be the reason, but I have no idea why it worked before:

https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac

weslly · 2018-02-02T21:00:37Z

src/autotype/AutoTypeSelectDialog.cpp

@@ -82,20 +83,20 @@ void AutoTypeSelectDialog::done(int r)
    QDialog::done(r);
 }



This file needs a keyPressEvent() method to catch the return/enter key on macOS and emit the activated signal:

Strangely enought, this is already implemented in the EntryView class

keepassxc/src/gui/entry/EntryView.cpp

Line 115 in c4b9d54

void EntryView::keyPressEvent(QKeyEvent* event)

The EntryView (on Mac) it's emitting the activated signal and the autotypeSelectDialog is connected to it. I don't see why it's not working 🤔

Edit: got it, preparing the fix

Edit: Now I've figure out why it worked before. Now the signal it's called emitMatchActivated and the EntryView is emitting the emitEntryActivated one :D

@TheZ3ro nice!

TheZ3ro · 2018-02-04T22:34:20Z

@weslly opted for a different fix, can you test it out?

weslly · 2018-02-04T22:45:53Z

@TheZ3ro just tested, working fine now :)

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

TheZ3ro added user interface feature: Auto-Type labels Jan 16, 2018

TheZ3ro added this to the v2.3.0 milestone Jan 16, 2018

TheZ3ro requested a review from a team January 16, 2018 21:11

TheZ3ro mentioned this pull request Jan 23, 2018

Allow AutoType for only username or password #878

Closed

TheZ3ro force-pushed the feature/multiple-autotype-matches branch from 1c3bf07 to 354727c Compare January 24, 2018 21:50

weslly self-requested a review January 25, 2018 18:15

phoerious requested changes Jan 25, 2018

View reviewed changes

TheZ3ro force-pushed the feature/multiple-autotype-matches branch 2 times, most recently from d9f4ca8 to 0f42bfb Compare January 29, 2018 18:46

TheZ3ro mentioned this pull request Jan 29, 2018

Autotype fails and triggers lockup #975

Closed

phoerious approved these changes Jan 30, 2018

View reviewed changes

TheZ3ro force-pushed the feature/multiple-autotype-matches branch from 0f42bfb to a9b9575 Compare January 31, 2018 20:55

phoerious changed the base branch from develop to release/2.3.0 February 1, 2018 20:41

weslly reviewed Feb 2, 2018

View reviewed changes

weslly suggested changes Feb 2, 2018

View reviewed changes

TheZ3ro added 7 commits February 4, 2018 23:13

fix effective autotype sequence

065a85e

add support for multiple autotype sequence, fix #559

b5cabbe

refactor autotype sequences and entry-point functions

a9479fd

reorder functions by logic

12a4b9a

improve Window Associations UI/UX

ba4ef52

change inAutotype logic, preventing multiple autotype call

a76c92e

convert inAutoType from boolean block to QMutex

b4cf989

TheZ3ro added 2 commits February 4, 2018 23:13

fix duplicate autotype sequences

16fad1a

fix MatchView activation with Enter/Return on macOS

aa54c7b

TheZ3ro force-pushed the feature/multiple-autotype-matches branch from a9b9575 to aa54c7b Compare February 4, 2018 22:33

weslly approved these changes Feb 4, 2018

View reviewed changes

TheZ3ro merged commit 4206fd9 into release/2.3.0 Feb 5, 2018

TheZ3ro deleted the feature/multiple-autotype-matches branch February 5, 2018 10:09

TheZ3ro mentioned this pull request Jul 1, 2018

Auto-Type URL Matching of Window Title is not strict enough #2068

Closed

droidmonkey mentioned this pull request Jan 2, 2019

[Feature Request] Selective Auto-Type without duplicating entry #736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple autotype sequences for a single entry #1390

Add support for multiple autotype sequences for a single entry #1390

TheZ3ro commented Jan 16, 2018 •

edited

Loading

TheZ3ro commented Jan 18, 2018

phoerious Jan 25, 2018

TheZ3ro Jan 26, 2018

phoerious Jan 26, 2018

TheZ3ro Jan 29, 2018

phoerious Jan 29, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

TheZ3ro Jan 26, 2018

phoerious Jan 26, 2018 •

edited

Loading

phoerious Jan 25, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

phoerious Jan 25, 2018

weslly commented Jan 30, 2018 •

edited

Loading

TheZ3ro commented Jan 30, 2018

weslly commented Jan 31, 2018

TheZ3ro commented Jan 31, 2018

weslly commented Feb 1, 2018

weslly commented Feb 1, 2018

weslly Feb 2, 2018

weslly Feb 2, 2018

TheZ3ro Feb 4, 2018 •

edited

Loading

weslly Feb 4, 2018

TheZ3ro commented Feb 4, 2018

weslly commented Feb 4, 2018

		@@ -82,20 +83,20 @@ void AutoTypeSelectDialog::done(int r)
		QDialog::done(r);
		}

Add support for multiple autotype sequences for a single entry #1390

Add support for multiple autotype sequences for a single entry #1390

Conversation

TheZ3ro commented Jan 16, 2018 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

TheZ3ro commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weslly commented Jan 30, 2018 • edited Loading

TheZ3ro commented Jan 30, 2018

weslly commented Jan 31, 2018

TheZ3ro commented Jan 31, 2018

weslly commented Feb 1, 2018

weslly commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Feb 4, 2018

weslly commented Feb 4, 2018

TheZ3ro commented Jan 16, 2018 •

edited

Loading

phoerious Jan 26, 2018 •

edited

Loading

weslly commented Jan 30, 2018 •

edited

Loading

TheZ3ro Feb 4, 2018 •

edited

Loading