QR Code generator #1167

adolfogc · 2017-11-04T19:30:42Z

Description

This PR is an alternative to PR #1001.

Motivation and context

This is an improved version of PR #1001 (it also uses libqrencode, instead of qrcodegen).

How has this been tested?

Manually on macOS.

Screenshots (if appropriate):

Types of changes

✅ 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]
✅ All new and existing tests passed. [REQUIRED]
✅ 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.~~
~~☑️ I have added tests to cover my changes.~~

adolfogc · 2017-11-18T21:34:00Z

Having finished to organize the GIT history for this PR into a better set of commits, I consider it to be ready for review.

weslly · 2017-11-22T03:48:27Z

I'm not sure about the auto-close feature. An user with a bad camera on an old phone may take more than 30 seconds trying to focus a more detailed QR code like the one in the screenshot.

adolfogc · 2017-11-22T04:20:35Z

@weslly How much time would be reasonable? 60 seconds?

lenucksi · 2017-11-22T08:38:50Z

@adolfogc how about adding a configuration setting for it. I guess 45 seconds should work with most phones, so a default and maybe a few common options would be nice?

weslly · 2017-11-22T09:01:38Z

45 seconds is fine for most modern phones but I would prefer not having the auto-close. I think most users would just click the close button before the timer runs out when they are done.

There could also be cases where people open the QR code dialog before getting their phone ready and would have to open it again.

weslly · 2017-11-22T09:22:09Z

@adolfogc Maybe we could have something like this instead:

adolfogc · 2017-11-22T18:00:54Z

@lenucksi @weslly I'll modify the auto-close feature as follows:

use 45 seconds by default
allow the user to disable it by using the checkbox located at the left side of the text (this checkbox will affect the setting globally, not just for the entry for which the QR code is shown)

weslly · 2017-11-22T18:02:27Z

@adolfogc as long as the user can disable it, you can leave at 30s

adolfogc · 2017-11-27T05:46:32Z

@lenucksi @weslly The requested changes have been made. I agree it's more practical to auto-close after 45s, instead of after 30s. The motivation for the auto-close feature is allowing the DB to be locked when KeePassXC is left unattended.

weslly · 2017-11-29T21:46:56Z

@adolfogc The app crashes when I try to view the QR code from entries that use the alternative settings format from KeeOtp (key={secret}&size=6&step=30 at the otp attribute)

weslly · 2017-11-29T21:18:19Z

src/gui/MainWindow.ui

@@ -545,6 +546,11 @@
    <string>Show TOTP</string>
   </property>
  </action>
+  <action name="actionEntryTotpDisplayKey">
+   <property name="text">
+    <string>Show TOTP key</string>


Something like "Export TOTP settings" would be better here, since someone could confuse the key as the generated code.

adolfogc · 2017-11-30T00:27:04Z

@weslly Thanks for the review, I'll work on it this weekend.

TheZ3ro · 2017-12-10T14:26:44Z

I was looking into this, seems that libqrcodegen it's not in debian repository but I'm not sure. If it is we will have some problem when packaging for debian since we should use debian version of the library (that uses exception)

adolfogc · 2017-12-10T14:40:32Z

@TheZ3ro There is no dependency on an external library such as libqrencode (this was my previous attempt). All the code that is needed was placed in the src/qrcodegen directory; for QR code generation we're using a stripped-down version of Nayuki's QR Code Generator library (qrcodegen), modified to work without raising exceptions.

TheZ3ro · 2017-12-10T15:17:00Z

@adolfogc You are including code from another library (qrcodegen) and modifying it for our needs. It's fine from a license point of view, but debian packaging require us to use system library (if present) instead of bundling the library ourselves.

My comment was just an heads-up, currently qrcodegen doesn't seems to be in debian repo so it's fine, but if in the future it will be included as a separate package we will have some problem.
(I don't understand this clearly, TBH)

Maybe @hifi or @julian-klode can drive us the right way :)

julian-klode · 2017-12-10T15:24:32Z

Well, yeah, we don't want embedded code in Debian. If you want to use a library, and want to ship a convience bundle, it should be unmodified so we can link it against a system library. This mostly becomes a problem once multiple programs use the same library and the library gets a security update - it's a question of updating one package vs multiple (and keeping track of these).

Also the COPYING stuff is wrong. You added a GPL-3 license to the files, so they are not MIT licensed, but MIT and (GPL-2 or GPL-3). And while it's legal to add GPLed modifications like that, it's just rude IMO.

hifi · 2017-12-10T15:31:56Z

I would probably use a QR code generation library that's well supported across distributions rather than embed if possible.

qrencode as previously used seems like something you'd want to link against. Why was it changed to an embedded one?

https://packages.debian.org/stretch/qrencode
https://apps.fedoraproject.org/packages/qrencode

adolfogc · 2017-12-10T16:12:21Z

Hi guys, my reply below.

...but debian packaging require us to use system library (if present) instead of bundling the library ourselves.

This mostly becomes a problem once multiple programs use the same library and the library gets a security update - it's a question of updating one package vs multiple (and keeping track of these).

Nayuki's QR Code Generator library is relatively new and currently it's distributed as source code (please check its repo here: https://github.com/nayuki/QR-Code-generator). What is the best way to use this library as it is today?

Also the COPYING stuff is wrong. You added a GPL-3 license to the files, so they are not MIT licensed, but MIT and (GPL-2 or GPL-3). And while it's legal to add GPLed modifications like that, it's just rude IMO.

I don't have much experience with licensing stuff, please understand I wasn't trying to be rude. I just used the same license as the rest of the KeepassXC codebase; can change it to MIT if it's OK with the KeepassXC developers.

...qrencode as previously used seems like something you'd want to link against. Why was it changed to an embedded one?

Well, originally I used libqrencode but someone pointed to Nayuki's QR code implementation and I though it was a nice alternative. Two reasons for the change were: keeping the sensitive information
(i.e. keys) inside KeePassXC and not requiring an external dependency. Also, its codebase is smaller and thus easier to inspect / audit eventually.

droidmonkey · 2017-12-11T01:25:21Z

The Debian comment mainly related to using distributed libraries to prevent security issues (ie, http).. the qrcode library doesn't fall into that camp.

TheZ3ro · 2017-12-11T06:29:51Z

Personally I will prefer a well supported library like qrencode, less code to maintain ourselves in the core codebase

adolfogc · 2018-01-22T18:37:05Z

@phoerious Thanks, that fixed the issue I was having with CI.

@hifi I tried fixing the aspect ratio issue by subclassing QSvgWidget and overriding its hasHeightForWidth/0 and heightForWidth/1 methods, it didn't work though. Do you have a suggestion on how to fix this?

hifi · 2018-01-23T03:59:57Z

@adolfogc I think you need to have a layout that allows space around the widget (with spacers?) so that something will fill the window when it's not a perfect square.

DJCrashdummy · 2018-03-13T12:15:20Z

just my 2 cents about the auto-close feature:

what about a line at the general security-settings in the timeout-section, to uncheck - this should be activated by default - an option to "simultaneously close the TOTP QR-code window with the database" and then simply close it a second before the whole database gets locked?! ...or maybe even hardcode this behavior?
--> this ensures that the auto-close timeouts match (e.g. i configured my database to close after 20sec, so 45sec for the qr-code doesn't make sense).
and maybe add an additional line similar to the clipboard-setting to optional auto-close the qr-code after a given time - in most cases earlier, which will make the first setting irrelevant, if used.
--> the question is, should there values higher than for the database-close be allowed or not...? - if yes, the first setting resp. its handling is not completely irrelevant.

the first setting (resp. its default behavior) will prevent conflicts or headaches for newbies and placing the setting there will make it easier to understand that the auto-close of the qr-code is due to security reasons.

adolfogc · 2018-03-26T04:56:38Z

@DJCrashdummy Currently, you can set this value as shown below:

the question is, should there values higher than for the database-close be allowed or not...? - if yes, the first setting resp. its handling is not completely irrelevant.

Though one would expect the timeout used for the DB auto-close to be higher than the one used for closing the QR code window, I guess it does make sense to use the minimum of those two timeout values, as you're suggesting. What about letting the user specify the timeout value and default to using min(dbAutoCloseTimeout, qrCodeWindowAutoCloseTimeout) for deciding when to close the QR code window?

DJCrashdummy · 2018-03-26T10:56:53Z

I guess it does make sense to use the minimum of those two timeout values, as you're suggesting.

no, i'm rather suggesting an additional checkbox (if not hardcoded?) to ensure, that the qr-code window gets always closed when the database gets locked, whether because of timeout or anything else. so this would also apply to the window minimizing behavior, session-lock, lid-close etc.
--> IMHO this is an essential security-behavior, because the qr-code contains parts of my database!
this behavior must overrule the qr-timeout and so may but doesn't have to make the qr-timeout irrelevant...

AFTER this essential feature is implemented (and only if optional as checkbox), and both timeouts are activated, this question you are trying to answer arises.
...but then IMHO it wouldn't be that tragically to allow the qr-window to stay open although the database is already closed.

droidmonkey · 2018-04-03T02:07:34Z

@adolfogc can you rebase this on develop? Now that the code format is merged we are ready to merge 2.4 PR's.

adolfogc · 2018-04-10T17:42:44Z

@droidmonkey I'll try to work on it this weekend.

droidmonkey · 2018-06-11T02:40:06Z

@adolfogc @phoerious @hifi do you think this needs anymore work?

TheZ3ro · 2018-07-03T19:50:12Z

Status on this PR?

adolfogc · 2018-07-10T04:30:25Z

@droidmonkey, @TheZ3ro: Sorry for the late reply guys; due to work, I haven't had the chance to work on this PR lately. There are still some changes I'd like to make.

droidmonkey · 2018-08-26T16:05:35Z

Hello @adolfogc, will you be able to make your changes? I would love to merge this in to develop. If not, can you describe what you want to change and maybe someone else can do it?

I rebased onto develop for you.

droidmonkey

I also recommend moving the "Show QR Code" into the "Show TOTP" dialog which would then open the QR Code dialog.

droidmonkey · 2018-08-26T16:19:24Z

src/gui/SettingsWidget.cpp

+    m_secUi->closeTotpExportSettingsDialogCheckBox->setChecked(
+        config()->get("security/AutoCloseTotpExportSettingsDialog", true).toBool());
+    m_secUi->closeTotpExportSettingsDialogSpinBox->setValue(
+        config()->get("security/AutoCloseTotpExportSettingsDialogTimeout", 45).toInt());


I personally think this setting is unnecessary. Just fix it at 45 seconds. Will make this PR more streamlined.

Changes made

adolfogc · 2018-09-03T04:15:03Z

Hi @droidmonkey, sorry for the late reply. Thanks for the improvements you made in cdd26c3! I'm currently unable to work on this PR, so I was wondering if perhaps you can finish it by implementing these changes:

remove the unnecessary setting and fix the timeout to 45 seconds, as you suggested
close the QR code dialog automatically when the DB gets locked, as suggested by @DJCrashdummy

- Improved version of PR #1001. - Uses libqrencode instead of qrcodegen. - Refer to PR #1001 and PR #1167 for extra details.

droidmonkey · 2018-10-18T12:13:28Z

This is ready for rebase merge.

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

adolfogc changed the title ~~[WIP, RFC] Alternative implementation for feature/764~~ [WIP, RFC] Alternative implementation for feature #764 Nov 4, 2017

adolfogc mentioned this pull request Nov 4, 2017

Option to display data as QR code #675

Open

droidmonkey changed the title ~~[WIP, RFC] Alternative implementation for feature #764~~ QR Code generator Nov 5, 2017

droidmonkey requested review from droidmonkey, weslly and phoerious November 5, 2017 03:30

droidmonkey added new feature user interface labels Nov 5, 2017

droidmonkey added this to the v2.3.0 milestone Nov 5, 2017

adolfogc mentioned this pull request Nov 5, 2017

Add support for displaying TOTP keys as QR codes #1001

Closed

weslly reviewed Nov 29, 2017

View reviewed changes

droidmonkey approved these changes Jan 27, 2018

View reviewed changes

droidmonkey reviewed Aug 26, 2018

View reviewed changes

droidmonkey mentioned this pull request Sep 2, 2018

recovery keys for OTP [feature] #2263

Closed

droidmonkey self-assigned this Sep 25, 2018

adolfogc and others added 4 commits October 18, 2018 08:11

Add feature #764

4133f8c

- Improved version of PR #1001. - Uses libqrencode instead of qrcodegen. - Refer to PR #1001 and PR #1167 for extra details.

Increase Dockerfile counter

bf704aa

Ensure QR code remains square & code cleanup

e871ac1

Close QRCode dialog when database is locked

cfb9e7a

droidmonkey added the PR: Ready for Merge label Oct 18, 2018

droidmonkey added 2 commits October 18, 2018 17:20

Add qtsvg to docker files

f6ec150

Correct otpauth URI output

3830bef

droidmonkey merged commit bb16dc6 into keepassxreboot:develop Oct 19, 2018

droidmonkey removed the PR: Ready for Merge label Oct 19, 2018

childnode mentioned this pull request Oct 8, 2019

TOTP: OTP settings not updated if in invalid format [DATA LOSS] #3142

Closed

QR Code generator #1167

QR Code generator #1167

Conversation

adolfogc commented Nov 4, 2017 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

adolfogc commented Nov 18, 2017

weslly commented Nov 22, 2017

adolfogc commented Nov 22, 2017

lenucksi commented Nov 22, 2017

weslly commented Nov 22, 2017

weslly commented Nov 22, 2017

adolfogc commented Nov 22, 2017 • edited Loading

weslly commented Nov 22, 2017

adolfogc commented Nov 27, 2017

weslly commented Nov 29, 2017

weslly Nov 29, 2017

Choose a reason for hiding this comment

adolfogc commented Nov 30, 2017

TheZ3ro commented Dec 10, 2017

adolfogc commented Dec 10, 2017 • edited Loading

TheZ3ro commented Dec 10, 2017 • edited Loading

julian-klode commented Dec 10, 2017

hifi commented Dec 10, 2017

adolfogc commented Dec 10, 2017 • edited Loading

droidmonkey commented Dec 11, 2017

TheZ3ro commented Dec 11, 2017

adolfogc commented Jan 22, 2018 • edited Loading

hifi commented Jan 23, 2018

DJCrashdummy commented Mar 13, 2018 • edited Loading

adolfogc commented Mar 26, 2018 • edited Loading

DJCrashdummy commented Mar 26, 2018

droidmonkey commented Apr 3, 2018

adolfogc commented Apr 10, 2018

droidmonkey commented Jun 11, 2018 • edited Loading

TheZ3ro commented Jul 3, 2018

adolfogc commented Jul 10, 2018

droidmonkey commented Aug 26, 2018 • edited Loading

droidmonkey left a comment

Choose a reason for hiding this comment

droidmonkey Aug 26, 2018

Choose a reason for hiding this comment

adolfogc commented Sep 3, 2018

droidmonkey commented Oct 18, 2018

adolfogc commented Nov 4, 2017 •

edited

Loading

adolfogc commented Nov 22, 2017 •

edited

Loading

adolfogc commented Dec 10, 2017 •

edited

Loading

TheZ3ro commented Dec 10, 2017 •

edited

Loading

adolfogc commented Dec 10, 2017 •

edited

Loading

adolfogc commented Jan 22, 2018 •

edited

Loading

DJCrashdummy commented Mar 13, 2018 •

edited

Loading

adolfogc commented Mar 26, 2018 •

edited

Loading

droidmonkey commented Jun 11, 2018 •

edited

Loading

droidmonkey commented Aug 26, 2018 •

edited

Loading