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 support for multiple URL's in entry #3558

Merged

Conversation

@varjolintu
Copy link
Member

varjolintu commented Sep 19, 2019

Type of change

  • New feature (non-breaking change which adds functionality)

Description and Context

Adds support for multiple URL's per entry. The new Browser Integration entry settings page has a list view with any additional URL's. These URL's are added to the entry attributes with KP2A_URL_<counter>, which means those are directly compatible with Keepass2Android.

Please note that #3444 (Browser Integration releated entry settings) must be merged before this PR. This extends that feature. Alternatively, that PR can be dismissed totally and all the features will be included in this one.

Fixes #398.

Screenshots

The Browser Integration page shows the following list:
Screen Shot 2019-09-19 at 12 39 41

The URL's will be also added to the attributes page:
Screen Shot 2019-09-19 at 12 39 52

Testing strategy

Manually. GUI tests are also included.

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 varjolintu added this to the v2.5.0 milestone Sep 19, 2019
@varjolintu varjolintu requested a review from keepassxreboot/core-developers Sep 19, 2019
Copy link
Member

droidmonkey left a comment

Quick review, can't wait to try it out

src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
src/gui/entry/EditEntryWidget.cpp Outdated Show resolved Hide resolved
src/gui/entry/EditEntryWidget.cpp Show resolved Hide resolved
src/gui/entry/EditEntryWidget.cpp Outdated Show resolved Hide resolved
@varjolintu varjolintu force-pushed the varjolintu:feature/multiple_urls branch from ca9dfe8 to 772a8af Sep 21, 2019
@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 21, 2019

@droidmonkey Fixes made.

@yan12125 yan12125 referenced this pull request Sep 22, 2019
yan12125 added a commit to yan12125/keepassxc that referenced this pull request Sep 28, 2019
yan12125 added a commit to archlinuxcn/repo that referenced this pull request Sep 29, 2019
Copy link
Contributor

yan12125 left a comment

Thanks for this PR! Just a minor note.

src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
@yan12125

This comment has been minimized.

Copy link
Contributor

yan12125 commented Sep 30, 2019

An untested diff for my previous comment:

diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp
index eb4c32b3..edcd793e 100644
--- a/src/browser/BrowserService.cpp
+++ b/src/browser/BrowserService.cpp
@@ -320,7 +320,7 @@ QString BrowserService::storeKey(const QString& key)
             return {};
         }

-        contains = db->metadata()->customData()->contains(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + id);
+        contains = db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + id);
         if (contains) {
             dialogResult = MessageBox::warning(nullptr,
                                                tr("KeePassXC: Overwrite existing key?"),
@@ -333,7 +333,7 @@ QString BrowserService::storeKey(const QString& key)
     } while (contains && dialogResult == MessageBox::Cancel);

     hideWindow();
-    db->metadata()->customData()->set(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + id, key);
+    db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + id, key);
     return id;
 }

@@ -344,7 +344,7 @@ QString BrowserService::getKey(const QString& id)
         return {};
     }

-    return db->metadata()->customData()->value(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + id);
+    return db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + id);
 }

 QJsonArray BrowserService::findMatchingEntries(const QString& id,
@@ -651,7 +651,7 @@ QList<Entry*> BrowserService::searchEntries(const QString& url, const StringPair
                     // Check if database is connected with KeePassXC-Browser
                     for (const auto& keyPair : keyList) {
                         QString key =
-                            db->metadata()->customData()->value(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + keyPair.first);
+                            db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + keyPair.first);
                         if (!key.isEmpty() && keyPair.second == key) {
                             databases << db;
                         }
@@ -741,12 +741,9 @@ void BrowserService::convertAttributesToCustomData(const QSharedPointer<Database
         return;
     }

-    const QString keePassBrowserGroupName = QLatin1String(KEEPASSXCBROWSER_GROUP_NAME.toUtf8());
-    const QString keePassHttpGroupName = QLatin1String(KEEPASSHTTP_GROUP_NAME.toUtf8());
-
     for (auto* g : rootGroup->groupsRecursive(true)) {
-        if (g->name() == keePassHttpGroupName) {
-            g->setName(keePassBrowserGroupName);
+        if (g->name() == KEEPASSHTTP_GROUP_NAME) {
+            g->setName(KEEPASSXCBROWSER_GROUP_NAME);
             break;
         }
     }
@@ -919,17 +916,15 @@ Group* BrowserService::getDefaultEntryGroup(const QSharedPointer<Database>& sele
         return nullptr;
     }

-    const QString groupName = QLatin1String(KEEPASSXCBROWSER_GROUP_NAME.toUtf8());
-
     for (auto* g : rootGroup->groupsRecursive(true)) {
-        if (g->name() == groupName && !g->isRecycled()) {
+        if (g->name() == KEEPASSXCBROWSER_GROUP_NAME && !g->isRecycled()) {
             return db->rootGroup()->findGroupByUuid(g->uuid());
         }
     }

     auto* group = new Group();
     group->setUuid(QUuid::createUuid());
-    group->setName(groupName);
+    group->setName(KEEPASSXCBROWSER_GROUP_NAME);
     group->setIcon(KEEPASSXCBROWSER_DEFAULT_ICON);
     group->setParent(rootGroup);
     return group;
@@ -1100,8 +1095,8 @@ int BrowserService::moveKeysToCustomData(Entry* entry, const QSharedPointer<Data
             publicKey.remove(LEGACY_ASSOCIATE_KEY_PREFIX);

             // Add key to database custom data
-            if (db && !db->metadata()->customData()->contains(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + publicKey)) {
-                db->metadata()->customData()->set(QLatin1String(ASSOCIATE_KEY_PREFIX.toUtf8()) + publicKey,
+            if (db && !db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + publicKey)) {
+                db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + publicKey,
                                                   entry->attributes()->value(key));
                 ++keyCounter;
             }

By the way, could you rebase this PR against the develop branch?

@droidmonkey droidmonkey force-pushed the varjolintu:feature/multiple_urls branch from 772a8af to 1f32c6c Oct 5, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 5, 2019

I rebased, this could use a bit of testing before merge. Please give me thumbs up when ready.

@droidmonkey droidmonkey force-pushed the varjolintu:feature/multiple_urls branch from 1f32c6c to 7671697 Oct 6, 2019
src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 7, 2019

Could we integrate this with the main edit entry widget? I think it makes more sense to support multiple URLs in general than to support it only for browser integration.

Also, I don't really like where this is going with all the vendor prefixes for custom attributes. Should we be initiating a joint effort with other programs to agree on an extended set of attributes with a common prefix? Otherwise this will get very chaotic. There's a reason why CSS got rid of vendor prefixes.

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 7, 2019

@phoerious We could. If we are going to do that, do we maintain the current UI or how are we going to display/add/modify/delete the additional URL's in the main edit entry widget?

I agree that a common prefix with other programs would be a much better choice. AFAIK the only application supporting multiple URL's at this point is KP2A, so it makes sense (at least for me) to use that prefix for now, even if it's a custom one.

@yan12125

This comment has been minimized.

Copy link
Contributor

yan12125 commented Oct 9, 2019

AFAIK the only application supporting multiple URL's at this point is KP2A

Another one that supports multiple URLs in an entry is Kee (formerly KeeFox). During the last time I checked (two years ago), it uses a quite different, JSON-based format as I described in #398 (comment).

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 9, 2019

@yan12125 Thanks. Good to know. It seems a bit complex, if the main function is just to provide an additional URL('s).

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 14, 2019

No JSON, please. The KDBX format is very flexible in what attributes it allows and the only difference between "main" attributes and "custom" attributes is that the GUI knows how to interpret the "main" ones. We should make use of that.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 16, 2019

So are we good with this for now? We can always work with KeePass2Android to define a more common method. Ideally this would also be a starting point to a better entry template format as well.

@varjolintu varjolintu force-pushed the varjolintu:feature/multiple_urls branch from 6f06d97 to e1a84b2 Oct 16, 2019
@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 16, 2019

Rebased.

* Fixes #398

The new Browser Integration entry settings page has a list view with any additional URL's. These URL's are added to the entry attributes with KP2A_URL_<counter>, which means those are directly compatible with Keepass2Android.
@droidmonkey droidmonkey force-pushed the varjolintu:feature/multiple_urls branch from e1a84b2 to af335c2 Oct 17, 2019
@droidmonkey droidmonkey merged commit f726d75 into keepassxreboot:develop Oct 17, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@varjolintu varjolintu deleted the varjolintu:feature/multiple_urls branch Oct 17, 2019
phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.