Show warning for legacy keyfile usage #1327
Conversation
phoerious
changed the title
phoerious
force-pushed
the
feature/warning-legacy-keyfile
branch
3 times, most recently
from
73f0711 to
eb7fd7d
Compare
| @@ -28,10 +28,19 @@ class QIODevice; | |||
| class FileKey: public Key | |||
| { | |||
| public: | |||
| enum Type { | |||
There was a problem hiding this comment.
How would you feel about using a scoped enum here? Their usage may be more verbose, e.g., FileKey::Type::Hashed instead of FileKey::Hashed, but they are safer as they prevent implicit conversion to their underlying integral type in comparison operations.
There was a problem hiding this comment.
I don't think the additional namespace encapsulation would remedy the additional typing overhead here. For a larger class with more members and possibly different enum types, I would totally agree with using class enums, but here I don't think it'd have any benefit over traditional eums. On the contrary, I find FileKey::Hashed to be semantically sensible.
There was a problem hiding this comment.
It's not just the additional namespace encapsulation though; the safer part comes from preventing implicit type conversion.
There was a problem hiding this comment.
True. But what would you compare these enums with that could cause problems?
There was a problem hiding this comment.
In practice, it can be an integral type, such as an int.
There was a problem hiding this comment.
Sure, but what would be the problem here? Where would you compare it with something so that automatic type conversion could fuck you up? And I don't mean in general (yes, in general that's a bad thing). I mean in this specific case.
There was a problem hiding this comment.
I get your point, in this particular case, and because code reviews are practiced, it would be difficult for such an issue to occur.
| @@ -82,6 +82,13 @@ int Extract::execute(QStringList arguments) | |||
| return EXIT_FAILURE; | |||
| } | |||
|
|
|||
| if (fileKey.type() != FileKey::Hashed) { | |||
| errorTextStream << QObject::tr("WARNING: You are using a legacy key file format which may become\n" | |||
There was a problem hiding this comment.
@phoerious you should use qWarn, since this is a warning message that will not abort the execution of the command. Also, you could remove the WARNING, and simplify the message by saying the legacy key file format is deprecated.
There was a problem hiding this comment.
qWarning() is a debug tool. This is not a debug warning. Also qWarning() messages aren't translatable, since it expects const char*, while tr() returns a QString.
phoerious
force-pushed
the
feature/warning-legacy-keyfile
branch
from
eb7fd7d to
87a840f
Compare
phoerious
force-pushed
the
feature/warning-legacy-keyfile
branch
from
87a840f to
082be4a
Compare
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]
Description
This is a follow-up PR to #1326, which adds a warning when using legacy key file formats.
Motivation and context
When we plan to deprecate old key file formats, we should tell users about it.
How has this been tested?
Manually and with additional unit tests.
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]