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

Use custom data to save any browser integration related data #1497

Merged

Conversation

Projects
None yet
4 participants
@varjolintu
Copy link
Member

varjolintu commented Feb 19, 2018

Description

Browser connection keys and Allow/Deny rules are stored to custom data instead of attributes.

Motivation and context

Fixes keepassxreboot/keepassxc-browser#11.

How has this been tested?

Tested manually with the latest KeePassXC-Browser.

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

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 20, 2018

This needs to be rebased and cleaned up after the Custom Data PR is merged.

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch 2 times, most recently from d569e33 to 06fadb4 Feb 21, 2018

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Feb 21, 2018

I wonder if there should be some transitional code for those who have used the old implementation. Maybe even check if we are using KDBX 4 already and then convert all string attributes to custom data, otherwise, keep using string attributes.

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Feb 24, 2018

This PR still needs a way to view/delete the stored keys. So a little more work is needed.

@droidmonkey droidmonkey added this to the v2.3.0 milestone Feb 24, 2018

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from f1194d2 to fe5eb02 Feb 26, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Feb 26, 2018

Added the key view/removal to Browser Integration settings window and rebased.

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch 2 times, most recently from e873c4d to a53196b Feb 27, 2018

@phoerious phoerious modified the milestones: v2.3.0, 2.3.1 Feb 27, 2018

@varjolintu varjolintu changed the base branch from release/2.3.0 to develop Feb 28, 2018

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from a53196b to 60d8670 Feb 28, 2018

@varjolintu varjolintu changed the base branch from develop to release/2.3.1 Mar 3, 2018

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from 60d8670 to aad4cd9 Mar 3, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Mar 3, 2018

This feature will be added #1591 to the PR.

@phoerious phoerious modified the milestones: 2.3.1, v2.4.0 Mar 3, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 3, 2018

This also needs to convert existing attributes to custom data, or will you be supporting both?

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Mar 4, 2018

@droidmonkey I'll do a feature that converts both KeePassHTTP and KeePassXC-Browser attributes to custom data. I will not support both of them.

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from 1c7ee40 to 53165ea Mar 5, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Mar 5, 2018

Button added to Browser Integration settings page. Rebased.

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 26, 2018

@droidmonkey I agree. This should be asked automatically (but still leaving the manual button just in case). If the KeePassXC-Browser Settings attribute is found, the conversion popup dialog appears. I'll do the necessary fixes today.

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 26, 2018

I noticed that the keys are not converted to custom data, but instead still using the old KeePassXC-Browser entry method. Maybe this needs to be changed too.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 26, 2018

IMO the browser entry should be deleted and the attributes moved to the root folder custom data.

We can also store the default folder location (user choice) for new entries made from the plugin in that custom data. Let's think outside the box a little.

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from cc70244 to 64e4417 Sep 26, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 26, 2018

Key conversion done. (Please review the dialog text).

Every time database is opened, BrowserService class checks if there are any old attributes or old key entry. If found, a popup dialog is shown.

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from 64e4417 to c923453 Sep 27, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 27, 2018

Key count added to the popup dialog. Checking for legacy settings has been moved to another function.

@droidmonkey
Copy link
Member

droidmonkey left a comment

Side note: when I remove connection keys from the database, the browser plugin remains connected to the database until it is locked. If I remove a key, I expect the browser plugin to lose connection immediately.

if (QMessageBox::Yes != MessageBox::question(this,
tr("Delete the selected key?"),
tr("Do you really want to delete the selected key?\n"
"This may cause the affected plugins to malfunction."),

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

What does it mean to malfunction? I think the correct verbiage here is "This may prevent connection to the browser plugin."

Note that we warn when you remove an individual key, but when you click on "Disconnect all browsers" it just does it without any confirmation. Same with "Forget all remembered permissions" which is a little bit confusing, this button should probably say "Forget all site-specific settings on entries".

This comment has been minimized.

Copy link
@varjolintu

varjolintu Oct 4, 2018

Author Member

I agree. There should be confirmation for each of these actions.

tr("KeePassXC: Settings not available!"),
Database* db = getDatabase();
if (!db) {
QMessageBox::information(0, tr("KeePassXC: Settings not available!"),

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

Technically if you reach here you do not have an active database. I think this warning can be dropped.

return;
}

Database* db = m_dbTabWidget->currentDatabaseWidget()->database();

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

Why isn't "getDatabase()" used here?

Here is my problem with this settings dialog, application settings are meant to be global. So these buttons make no sense at all. Basically I have to have the correct database selected prior to hitting the settings button, which is an undiscoverable "feature".

Here are some options:

  1. Move everything below "Sort matching credentials..." into the database settings
  2. Add a drop-down selection list above the buttons that let's you select from the currently open databases.

To me, option 1 is the most acceptable because it makes sense. It is also where the KeeShare settings are going so that would be consistent.

This comment has been minimized.

Copy link
@varjolintu

varjolintu Oct 4, 2018

Author Member

A separate Browser Integration page should be added to the Database settings. Then it would be nice and clean.

auto dialogResult = QMessageBox::warning(nullptr,
tr("KeePassXC: Legacy browser integration settings detected"),
tr("Legacy browser integration settings have been detected.\n"
"Do you want to transfer the settings and stored keys to custom data?\n"

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 4, 2018

Member

I would just say "Do you want to upgrade the settings to the latest standard?\nThis is necessary to maintain compatibility with the browser plugin."

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch 2 times, most recently from 5465cbf to d382905 Oct 9, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 9, 2018

An update:

  • Moved database specific settings from Browser Intergration settings to Database settings
  • Added confirmation popup dialog to the buttons before removing or converting any data
  • Cleaned the code from Browser Integration side

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch 5 times, most recently from 1e5c684 to 0e2b789 Oct 9, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 10, 2018

A final update:

  • Added a warning message (and disabled the buttons etc.) to Database settings browser page when Browser Integration is disabled.

@droidmonkey droidmonkey force-pushed the varjolintu:feature/custom-data-browser branch from 0e2b789 to b9057b9 Oct 23, 2018

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from 58f27fc to 0e9a618 Oct 24, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Oct 24, 2018

One more update:

  • Only keys with a KeePassXC-Browser specific prefix are shown in the Database settings custom data list
  • Key prefix is stripped from the key string

@varjolintu varjolintu force-pushed the varjolintu:feature/custom-data-browser branch from 0e9a618 to 008f41a Oct 24, 2018

@droidmonkey droidmonkey force-pushed the varjolintu:feature/custom-data-browser branch from 008f41a to 76095d0 Oct 24, 2018

@droidmonkey droidmonkey merged commit efdb43d into keepassxreboot:develop Oct 24, 2018

0 of 2 checks passed

CodeFactor Reviewing...
Details
Ubuntu Linux (KeepassXC) TeamCity build started
Details

@varjolintu varjolintu deleted the varjolintu:feature/custom-data-browser branch Oct 24, 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.