[WIP] Create new KeepassHTTP KeyAcceptDialog #850

duk3luk3 · 2017-08-06T05:47:35Z

WIP - Creates a new dialog widget for accepting keys - needed for #838

This is a proof-of-concept for myself that I can make changes to KeePassXC and a first step towards making some big changes to the KeepassHTTP authorization key management in order to address #838 and #530 and my own usability / workflow itches as someone who generally works with multiple databases open at the same time.

Please give this change a quick look and tell me if this is a good direction to go in, as well as anything problematic in the code I've changed (as far as it isn't a temporary mock-up / stub)

Description

Planned changes:

Fix KeePassHTTP Encryption Key not recognized when multiple databases are open #530
Allow to select which database(s) to add a KeepassHTTP authorization key to. (UX improvements for "New key association request" #838)
Figure out if more information about the source of the authorization request can be taken from the KeepassHTTP connection and display it

Motivation and context

See #530, #838

How has this been tested?

Not yet, WIP changes

Screenshots (if appropriate):

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)
✅ New feature (non-breaking change which adds functionality)
✅ 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]
X All new and existing tests passed. [REQUIRED]
X I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
? My change requires a change to the documentation and I have updated it accordingly.
X I have added tests to cover my changes.

droidmonkey · 2017-08-06T16:11:41Z

This needs to jive with #608 as well. We are likely going to pull support for the HTTP protocol in a couple releases due to its insecurity.

I really like the dialog and modification though! It is sorely needed.

duk3luk3 · 2017-08-07T03:06:18Z

What extensions are going to replace chromeIPass and passIFox?

droidmonkey · 2017-08-07T03:34:45Z

The author of the new method actually created a new plugin as well.

duk3luk3 · 2017-08-07T03:44:59Z

I see - I checked this out, I'll keep it in mind. thanks.

As far as a quick check tells me, the "semantic model" of remote connections will remain the same so I should be fine with just continuing work on this and adapting it for any changes based on keepassxc-browser should be easy once that is merged.

duk3luk3 · 2017-08-08T13:41:45Z

I have a question: It seems that the DatabaseTabWidget manages all databases and gives the rest of the codebase access only to the currently open database (via DatabaseTabWidget::currentDatabaseWidget). Is this just a historical artifact due to multi-database support not being part of the original KeePassX, or is that an intentional part of the design of KeePassX(C)?

EDIT: In Service::searchEntries access to all databases is gained by grabbing the databases out of the tab widgets; isn't that pretty ugly?

duk3luk3 · 2017-08-09T06:17:37Z

I've now added a proper feature: All databases are listed in the dialog and keys are inserted into all selected databases.

It would be very nice to get a proper review of my changes at this point (of course keeping in mind that it's still WIP).

TheZ3ro · 2017-09-19T11:21:06Z

Should we postpone this to 2.3.0 ?

droidmonkey · 2017-09-19T15:42:55Z

I think so, it should be synced with the new browser integration. I'll look at this tonight.

droidmonkey · 2017-09-20T02:30:00Z

I've tested this and it works well, however if you do not enter a name in the top box it fails to find the database. Recommend adding an error if no name is supplied or use the name of the database by default. There is also commented out code and XXX comments that need to be removed or addressed.

droidmonkey

Make the changes in my previous comment.

duk3luk3 · 2017-09-21T04:03:44Z

Thank you for testing this, will check your feedback!

I will add some inline comments to the things I marked with XXX, because this is where I'm unsure how exactly C++ and Qt work.

duk3luk3 · 2017-09-21T04:06:03Z

src/http/KeyAcceptDialog.cpp

+        }
+    }
+
+    return resultList;


Is it okay to stack-allocate (? - I am not sure if that is what happens, and whether Qt transforms that into something else) resultList here and then return it to the caller, where the caller will simply let it go out of scope?

I.e. will the object be destroyed once it runs out of scope in the caller, and is this the proper way to do it or should this be done differently?

Answered on second question

duk3luk3 · 2017-09-21T04:07:54Z

src/http/Service.cpp

+
+//XXX
+//bool Service::attributeExists(QList<Entry*> entries, const QString &attribute)
+bool Service::attributeExists(QList<Entry*> entries, const QString attribute)


Is const QString attribute or const QString &attribute correct here?

Yes this is correct. The data you allocate as part of the list gets shared with the calling scope's new QList object.

duk3luk3 · 2017-09-21T11:10:27Z

I rebased for now. To resolve to "TODOs", I still need an answer to my question upthread.

droidmonkey · 2017-09-21T22:27:11Z

I will be updating this behavior in my database pointer refactoring. Proceed with the ugly code for now.

duk3luk3 · 2017-09-23T02:30:47Z

if you do not enter a name in the top box it fails to find the database. Recommend adding an error if no name is supplied or use the name of the database by default

I've fixed this by not allowing to submit the dialog if the key name is empty. Are you happy with that solution?

duk3luk3 · 2017-09-24T12:03:46Z

I added some more cleanup and additional verification to the key accept dialog.

The only thing left to do for now is adding translation support.

duk3luk3 · 2017-09-25T12:09:41Z

Translation wrappers added.

@droidmonkey please review, then I will rebase and squash.

TheZ3ro · 2017-11-30T20:30:52Z

@keepassxreboot/core-developers can someone review this?

weslly

Coding style.

weslly · 2017-11-30T23:28:00Z

src/http/KeyAcceptDialog.cpp

+#include <QDialogButtonBox>
+#include <QPushButton>
+
+KeyAcceptDialog::KeyAcceptDialog(QWidget *parent) :


QWidget* parent

Pointers on left side.

weslly · 2017-11-30T23:33:07Z

src/http/KeyAcceptDialog.h

+    Q_OBJECT
+
+public:
+    explicit KeyAcceptDialog(QWidget *parent = nullptr);


QWidget* parent

weslly · 2017-11-30T23:34:00Z

src/http/KeyAcceptDialog.h

+
+private slots:
+    void keyEditChanged(const QString &text);
+    void modelItemChanged(QStandardItem *item);


QStandardItem* item

weslly · 2017-11-30T23:38:28Z

src/http/Service.cpp

-    if (DatabaseWidget* dbWidget = m_dbTabWidget->currentDatabaseWidget())
+    QList<Entry*> entries;
+
+    for (int i = 0; i < m_dbTabWidget->count(); i++)


This for block should be wrapped with braces.

That is inconsistent with the style of the rest of the file. Should I really partially fix the style of the file(s)? A separate code style commit would probably be better.

The same applies for NULL vs. nullptr.

That is inconsistent with the style of the rest of the file

Older code, mostly before the fork, don't necessarily follow the current coding style, but all new PR's should stick to the project coding style guide.

Should I really partially fix the style of the file(s)?

It's not required to fix the style where you didn't modify/add code, but we would appreciate if you do. :)

A separate code style commit would probably be better.

Concur.

Run this command to fix all the file codestyle to project standard
clang-format -i -style=file src/http/Service.cpp in the project root.

Yes, fix the codestyle in a new commit please

weslly · 2017-11-30T23:38:37Z

src/http/Service.cpp

+    QList<Entry*> entries;
+
+    for (int i = 0; i < m_dbTabWidget->count(); i++)
+        if (DatabaseWidget* dbWidget = qobject_cast<DatabaseWidget*>(m_dbTabWidget->widget(i)))


weslly · 2017-11-30T23:45:55Z

src/http/Server.h

@@ -44,7 +46,7 @@ class Server : public QObject
 public:
    explicit Server(QObject *parent = 0);

-    virtual bool isDatabaseOpened() const = 0;
+    virtual bool isDatabaseOpened(DatabaseWidget* dbWidget = NULL) const = 0;


Use nullptr instead of NULL.

weslly · 2017-11-30T23:46:24Z

src/http/Service.cpp

+                    if (create)
+                        entry->setGroup(db->rootGroup());
+                    else
+                        entry = NULL;


nullptr instead of NULL.

weslly · 2017-11-30T23:47:04Z

src/http/Service.cpp

@@ -522,7 +589,7 @@ QString Service::generatePassword()

 void Service::removeSharedEncryptionKeys()
 {
-    if (!isDatabaseOpened()) {
+    if (!isDatabaseOpened(NULL)) {


@duk3luk3 This one wasn't addressed

weslly · 2017-11-30T23:47:08Z

src/http/Service.cpp

@@ -561,7 +628,7 @@ void Service::removeSharedEncryptionKeys()

 void Service::removeStoredPermissions()
 {
-    if (!isDatabaseOpened()) {
+    if (!isDatabaseOpened(NULL)) {


@duk3luk3 This one wasn't addressed

weslly · 2017-11-30T23:53:39Z

src/http/KeyAcceptDialog.cpp

+        ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(false);
+        ui->buttonBox->button(QDialogButtonBox::Ok)->setToolTip(tr("Make sure the key name is not empty!"));
+    }
+    else if (!hasChecked) {


Remove newline between } and else if

droidmonkey · 2017-12-04T04:10:28Z

@duk3luk3 will you be submitting revisions for Weslley's comments?

duk3luk3 · 2017-12-05T23:52:02Z

Hi,

sure! Will do as soon as I am able.

duk3luk3 · 2017-12-12T09:29:24Z

@droidmonkey @weslly Please let me know if there are any other issues, and then I'll rebase the PR.

weslly · 2017-12-12T10:44:57Z

src/http/Service.cpp

@@ -522,7 +589,7 @@ QString Service::generatePassword()

 void Service::removeSharedEncryptionKeys()
 {
-    if (!isDatabaseOpened()) {
+    if (!isDatabaseOpened(NULL)) {


@duk3luk3 This one wasn't addressed

weslly · 2017-12-12T10:45:09Z

src/http/Service.cpp

@@ -561,7 +628,7 @@ void Service::removeSharedEncryptionKeys()

 void Service::removeStoredPermissions()
 {
-    if (!isDatabaseOpened()) {
+    if (!isDatabaseOpened(NULL)) {


@duk3luk3 This one wasn't addressed

duk3luk3 · 2017-12-12T11:05:55Z

@weslly Ah, I assumed clang-format would catch those. I amended the formatting commit.

TheZ3ro · 2017-12-12T11:32:37Z

@duk3luk3 can you rebase on top of develop? There are some conflict with src/http/Service.cpp

* !WIP! * Creates a new dialog widget for accepting keys - needed for keepassxreboot#838 For keepassxreboot#838, keepassxreboot#530

Basic implementation of allowing user to select which DBs to insert KeepassHTTP Authentication Keys into. For keepassxreboot#535

Disables the "OK" button when the key name edit is empty. This avoids saving a KeepassHTTP key with empty key name, which causes problems later.

TODO: Squash

To improve multi-db handling, we can move the checking for open (unlocked) databases to the key accept dialog. This means that even if the selected db is not open, the key can be stored in another db as selected by the user. This commit also adds a check for selected db's to the key accept dialog so the dialog cannot be submitted if no db's to store the key are selected.

Wrap strings presented to user in tr() for localization

Run clang-format and replace NULLs in all files touched by the PR

duk3luk3 · 2017-12-12T22:39:50Z

Rebased, test failures appear to come from develop: https://travis-ci.org/keepassxreboot/keepassxc/builds/315131994

I will do some more cleanup as soon as I am able.

TheZ3ro · 2018-01-04T22:49:47Z

@duk3luk3 can you please rebase again? we changed our CI

@droidmonkey can you please review your requested changes?
Thanks

duk3luk3 · 2018-01-04T23:43:05Z

I will have to do some more work now that #608 has been merged. Will do it asap.

TheZ3ro · 2018-01-04T23:59:28Z

No problem @duk3luk3. Ping me when you are done :)

droidmonkey · 2018-02-24T18:47:38Z

This PR has been overcome by the KeePassXC-Browser. HTTP is deprecated and will be removed in 2.4.0.

duk3luk3 · 2018-03-05T08:36:15Z

The changes in this PR should be 100% applicable to KeePassXC-Browser.

I just need to port them - I will do that as soon as I find time.

droidmonkey · 2018-03-06T04:49:14Z

I agree with that, thanks!

droidmonkey self-requested a review August 10, 2017 01:58

droidmonkey added feature: Browser user interface labels Aug 10, 2017

droidmonkey added this to the v2.2.1 milestone Aug 10, 2017

droidmonkey requested changes Sep 20, 2017

View reviewed changes

droidmonkey modified the milestones: v2.2.1, v2.3.0 Sep 20, 2017

duk3luk3 commented Sep 21, 2017

View reviewed changes

duk3luk3 force-pushed the feature/838-keepasshttp-keys branch from c513ffe to 2eec43e Compare September 21, 2017 11:04

duk3luk3 changed the title ~~[WIP, RFC] Create new KeepassHTTP KeyAcceptDialog~~ [WIP] Create new KeepassHTTP KeyAcceptDialog Sep 25, 2017

TheZ3ro requested a review from a team November 6, 2017 19:24

weslly suggested changes Nov 30, 2017

View reviewed changes

weslly suggested changes Dec 12, 2017

View reviewed changes

duk3luk3 force-pushed the feature/838-keepasshttp-keys branch from e74839b to e514437 Compare December 12, 2017 11:03

weslly approved these changes Dec 12, 2017

View reviewed changes

duk3luk3 added 7 commits December 13, 2017 09:17

Create new KeepassHTTP KeyAcceptDialog

0eb07ad

* !WIP! * Creates a new dialog widget for accepting keys - needed for keepassxreboot#838 For keepassxreboot#838, keepassxreboot#530

Insert KeepassHTTP Auth Key into multiple DBs

62dc9d8

Basic implementation of allowing user to select which DBs to insert KeepassHTTP Authentication Keys into. For keepassxreboot#535

Handle empty key name edit

840c04e

Disables the "OK" button when the key name edit is empty. This avoids saving a KeepassHTTP key with empty key name, which causes problems later.

Clean up

8562fb1

TODO: Squash

Make strings translateable

b860e4c

Wrap strings presented to user in tr() for localization

Formatting

e97dacb

Run clang-format and replace NULLs in all files touched by the PR

duk3luk3 force-pushed the feature/838-keepasshttp-keys branch from e514437 to e97dacb Compare December 12, 2017 22:24

Remove obsolete TODO comments

64689ae

phoerious added the needs work label Jan 14, 2018

duk3luk3 mentioned this pull request Jan 20, 2018

How to connect to recent keepassxc source build? keepassxreboot/keepassxc-browser#3

Closed

phoerious modified the milestones: v2.3.0, Future Jan 28, 2018

droidmonkey closed this Feb 24, 2018

droidmonkey removed this from the Future milestone Feb 24, 2018

[WIP] Create new KeepassHTTP KeyAcceptDialog #850

[WIP] Create new KeepassHTTP KeyAcceptDialog #850

Conversation

duk3luk3 commented Aug 6, 2017

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

droidmonkey commented Aug 6, 2017 • edited

duk3luk3 commented Aug 7, 2017

droidmonkey commented Aug 7, 2017

duk3luk3 commented Aug 7, 2017 • edited

duk3luk3 commented Aug 8, 2017 • edited

duk3luk3 commented Aug 9, 2017 • edited

TheZ3ro commented Sep 19, 2017

droidmonkey commented Sep 19, 2017

droidmonkey commented Sep 20, 2017 • edited

droidmonkey left a comment

Choose a reason for hiding this comment

duk3luk3 commented Sep 21, 2017

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

duk3luk3 commented Sep 21, 2017

droidmonkey commented Sep 21, 2017

duk3luk3 commented Sep 23, 2017

duk3luk3 commented Sep 24, 2017

duk3luk3 commented Sep 25, 2017 • edited

TheZ3ro commented Nov 30, 2017

weslly left a 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

duk3luk3 Dec 8, 2017 • edited

Choose a reason for hiding this comment

weslly Dec 8, 2017 • edited

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

droidmonkey commented Dec 4, 2017

duk3luk3 commented Dec 5, 2017

duk3luk3 commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duk3luk3 commented Dec 12, 2017

TheZ3ro commented Dec 12, 2017

duk3luk3 commented Dec 12, 2017

TheZ3ro commented Jan 4, 2018

duk3luk3 commented Jan 4, 2018

TheZ3ro commented Jan 4, 2018

droidmonkey commented Feb 24, 2018

duk3luk3 commented Mar 5, 2018

droidmonkey commented Mar 6, 2018

droidmonkey commented Aug 6, 2017 •

edited

duk3luk3 commented Aug 7, 2017 •

edited

duk3luk3 commented Aug 8, 2017 •

edited

duk3luk3 commented Aug 9, 2017 •

edited

droidmonkey commented Sep 20, 2017 •

edited

duk3luk3 commented Sep 25, 2017 •

edited

duk3luk3 Dec 8, 2017 •

edited

weslly Dec 8, 2017 •

edited