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

Hotfix/2657 prevent share overwrite #2746

Conversation

Projects
None yet
2 participants
@ckieschnick
Copy link
Contributor

commented Feb 27, 2019

Added warnings and prevented export when exports are conflicting, added warning when imports are using the same source.

Fixed an additional bug which prevented the usage of exports when a database was newly created.

Addresses an issue discussed in #2657.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Description and Context

Creation of a new database did not trigger the export even when needed references were defined.
In case of multiple usage of the same export/import container, there was no indication about that. Now the import/export and configuration gives warnings/errors about those cases.

Testing strategy

Manually tested.

Points to discuss

Currently the checking uses the entered paths. We may extend the checking to use canonical paths which may reduce (but not eliminate) the error rate.
An alternative (but does not prevent export!) would be to open and check the container for an export flag in the public data which indicates if the container was already written within the "transaction" and to abort the following exports for this container. I'm not sure if this would be a better solution to the problem since we do export data partially and (for now) there is no guarantee that the order of exports is deterministic.

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]

ckieschnick added some commits Feb 27, 2019

Fix problem with export from newly saved database
Newly created/saved databases (or used with DatabaseWidget::saveAs)
were not exported/imported correctly.
Fixed the problem by reinitializing the ShareObserver on
DatabaseWidget::saveAs.
Introduce warnings and prevent conflicting shares
Introduced several warnings and errors to indicate improper settings.
Prevent export when a path is used multiple times (only the file path is
checked - may ignore multiple similar ways to reference a share).
@droidmonkey
Copy link
Member

left a comment

Haven't looked at the rest, just caught some stuff

Show resolved Hide resolved src/gui/DatabaseWidget.cpp Outdated
Show resolved Hide resolved src/gui/DatabaseWidget.cpp Outdated
Improve KeeShare integration in DatabaseWidget
Moved initial KeeShare association to constructor.
Introduced Q_UNUSED to indicate need for assignment statement.
Show resolved Hide resolved src/gui/group/EditGroupWidget.h Outdated
Show resolved Hide resolved src/keeshare/group/EditGroupPageKeeShare.h Outdated
Show resolved Hide resolved src/keeshare/group/EditGroupWidgetKeeShare.h Outdated
Introduce shared pointer to pass db
EditGroupWidget passes a shared pointer to the database instead of a raw
pointer.
@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Concerning the shared pointer used by the EditGroupWidget and EditEntryWidget - is it possible that this pointer does extend the lifetime of the database pointer unnecessarily?

@droidmonkey

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Yes that is possible. You could use a QWeakPointer in this case.

@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I'd had a closer look at the source code. It seems, that there are a number of QSharedPointer (introduced before this pull request) which may also influence the lifetime of the object. EditWidgetIcons uses currently the same pattern.

Since it is already used without negative effects, I would suggest to keep the shared pointer for now and check if the change to QWeakPointer is necessary in a separate ticket. Maybe it can be combined with a general refactoring task to populate the settings widgets like EditEntryWidget with the specialized widgets fulfilling a simple interface (like already introduced for EditGroupWidget with KeeShare and DatabaseSettingsDialog).

@droidmonkey droidmonkey added this to the v2.4.0 milestone Mar 7, 2019

@droidmonkey droidmonkey merged commit 11ecaf4 into keepassxreboot:release/2.4.0 Mar 16, 2019

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

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.