Deactivate read-only mode autodetection. Fixes #803. #4508
Conversation
|
After reading your argument, use case, and reflecting on its purpose... I absolutely agree! Thank you for this submission. |
|
It looks like the failing MacOS test is also failing on the develop branch head, so I'll assume it's not me. |
|
I kind of disagree. Read-only mode serves the purpose of informing the user that the database has been opened from a device that does not support saving back changes. It will therefore not allow for any operations that would mark the database as dirty, avoiding confusion and a ton of annoying errors with autosave. It will also not show a pop-up dialogue on exit asking the user to save when the storage medium is read-only. |
|
On the other hand, (auto)save failures serve the same purpose of informing the user that the database isn't writeable. And the difference is whether they encounter an error when modifying a database, or when saving a modified database. If a database really "should" be read-only by policy, then it's still up to the user to understand the policy of where they should store new passwords. Without read-only mode, there's also the advantage that if the storage becomes writeable/non-writeable/inaccessible, the user's workflow adapts automatically, both with and without autosave. This use case is interesting to me since I want to store my database in google drive for now, and eventually in nextcloud when I get it set up, and cloud connectivity comes and goes. An automagic, sticky read-only mode will get in my way much more than it will help me. As I mentioned, the read-only mode workflow seems broken right now, and I don't see any open or closed issues related to read-only mode being broken, only issues about it triggering bugs. I took this as evidence that very few people actually use the workflow in the way you describe. I would really be happy to write a PR that satisfies both my use cases and those of the read-only mode workflow, but right now maybe I don't understand the workflow very well. |
|
There are many implicit changes that mark a DB as dirty. If you open a read-only database you should not see intermittent errors and you should not be able make any explicit changes in the first place unless you save the DB somewhere else. |
|
What are these implicit changes? Could you describe the use case further so I can understand? |
|
It's what we call ephemeral changes in the spec and it includes things like collapsing or expanding groups. Granted, we do not mark the DB dirty for that alone, but with autosave we totally could, probably leading to an overall better user experience (no more confusion about when KeePassXC remembers the status of groups and when it doesn't). Ideally, read-only mode should avoid confusion and prevent data loss. For instance, connecting the browser extension or saving browser extension access privileges (another rather implicit change) shouldn't work in read-only mode. I don't think this is implemented properly at the moment, but I'd rather fix that than remove read-only mode entirely. |
|
Hm, let me ask my question a different way. Is read-only mode something you envision as happening intentionally by policy, or something that happens "unintentionally", such as opening a database on a filesystem marked read-only due to some errors? If it's to be by policy, what is the administrator trying to do? |
|
I think we should keep read-only as an indicator only, not as something that would prevent changes to an open database. Modern programs, such as Word or Excel, open files in read-only all the time and when you go to save it pops up the save-as dialog instead. It does NOT prevent you from making changes though. Right now if saving fails 3 times in a row, the UI asks if you want to disable save safes. This is where not treating read-only as a "special case" can get you in trouble because you will inadvertently activate that code. Perhaps there is a better middle ground here. |
|
@droidmonkey If we make KeePassXC's read-only mode like Word (or vim) here, it doesn't solve #803 then. Certain filesystems will wrongly trigger save-as when opened and autosaved. @phoerious your point is also interesting. So a "strict" read-only mode would really mean that a user can't even expand or collapse groups, unless they save the database locally. This seems pretty confusing, as expanding/collapsing groups probably seems like a trivial UI issue to most users. Read-only mode seems like it could be hard to get right. I stand by my argument: evidence seems to say that many more people are stung by #803 than depend on read-only mode. What do you guys think about deactivating read-only mode for now to solve the immediate bug, then later we can come up with a good design for read-only mode later? I updated the PR to just "deactivate" read-only mode for now, keeping the rest of the code intact. I verified it still solves #803. |
sbrudenell
changed the title
|
Ping. Thoughts? |
No, but it would guarantee that none of these changes are modify the database.
I could live with that temporary solution, though I believe fixing the detection of read-only media would be the correct way to go. |
|
I am ok with this fix. If the database is truly read-only then the "Save As..." dialog will correctly show when trying to save. A valid alternative to auto-detecting read-only would be to explicitly open a database as read-only. We can introduce that at a later time. |
|
Might I ask which version/when this will be in? |
|
Look at the milestone 😉 |
|
Right you are, apologies. (But hey at least I'll get email updates about this now!) :) |
Read-only mode isn't in a good state and is triggering bugs. We'll come up with a good design for read-only mode at a later time.
|
Does this really need to wait for 2.6? It seems to be an "easily applied" fix that would rectify this long-standing issue for Linux users. Just asking ;) |
|
2.6 is the next release. |
|
Ah. That's great then.
…On Tue, 19 May 2020, 10:42 Janek Bevendorff, ***@***.***> wrote:
2.6 is the next release.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4508 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKS7WZSNQJD7WE6VNSA5E3RSJBBDANCNFSM4LVZYRUQ>
.
|
Added - Custom Light and Dark themes [#4110, #4769, #4791, #4796, #4892, #4915] - Compact mode to use classic Group and Entry line height [#4910] - View menu to quickly switch themes, compact mode, and toggle UI elements [#4910] - Search for groups and scope search to matched groups [#4705] - Save Database Backup feature [#4550] - Sort entries by "natural order" and move lines up/down [#4357] - Option to launch KeePassXC on system startup/login [#4675] - Caps Lock warning on password input fields [#3646] - Add "Size" column to entry view [#4588] - Browser-like tab experience using Ctrl+[Num] (Alt+[Num] on Linux) [#4063, #4305] - Password Generator: Define additional characters to choose from [#3876] - Reports: Database password health check (offline) [#3993] - Reports: HIBP online service to check for breached passwords [#4438] - Auto-Type: DateTime placeholders [#4409] - Browser: Show group name in results sent to browser extension [#4111] - Browser: Ability to define a custom browser location (macOS and Linux only) [#4148] - Browser: Ability to change root group UUID and inline edit connection ID [#4315, #4591] - CLI: `db-info` command [#4231] - CLI: Use wl-clipboard if xclip is not available (Linux) [#4323] - CLI: Incorporate xclip into snap builds [#4697] - SSH Agent: Key file path env substitution, SSH_AUTH_SOCK override, and connection test [#3769, #3801, #4545] - SSH Agent: Context menu actions to add/remove keys [#4290] Changed - Complete replacement of default database icons [#4699] - Complete replacement of application icons [#4066, #4161, #4203, #4411] - Complete rewrite of documentation and manpages using Asciidoctor [#4937] - Complete refactor of config files; separate between local and roaming [#4665] - Complete refactor of browser integration and proxy code [#4680] - Complete refactor of hardware key integration (YubiKey and OnlyKey) [#4584, #4843] - Significantly improve performance when saving and opening databases [#4309, #4833] - Remove read-only detection for database files [#4508] - Overhaul of password fields and password generator [#4367] - Replace instances of "Master Key" with "Database Credentials" [#4929] - Change settings checkboxes to positive phrasing for consistency [#4715] - Improve UX of using entry actions (focus fix) [#3893] - Set expiration time to Now when enabling entry expiration [#4406] - Always show "New Entry" in context menu [#4617] - Issue warning before adding large attachments [#4651] - Improve importing OPVault [#4630] - Improve AutoOpen capability [#3901, #4752] - Check for updates every 7 days even while still running [#4752] - Improve Windows installer UI/UX [#4675] - Improve config file handling of portable distribution [#4131, #4752] - macOS: Hide dock icon when application is hidden to tray [#4782] - Browser: Use unlock dialog to improve UX of opening a locked database [#3698] - Browser: Improve database and entry settings experience [#4392, #4591] - Browser: Improve confirm access dialog [#2143, #4660] - KeeShare: Improve monitoring file changes of shares [#4720] - CLI: Rename `create` command to `db-create` [#4231] - CLI: Cleanup `db-create` options (`--set-key-file` and `--set-password`) [#4313] - CLI: Use stderr for help text and password prompts [#4086, #4623] - FdoSecrets: Display existing secret service process [#4128] Fixed - Fix changing focus around the main window using tab key [#4641] - Fix search field clearing while still using the application [#4368] - Improve search help widget displaying on macOS and Linux [#4236] - Return keyboard focus after editing an entry [#4287] - Reset database path after failed "Save As" [#4526] - Use SHA256 Digest for Windows code signing [#4129] - Improve handling of ccache when building [#4104, #4335] - macOS: Properly re-hide application window after browser integration and Auto-Type usage [#4909] - Auto-Type: Fix crash when performing on new entry [#4132] - Browser: Send legacy HTTP settings to recycle bin [#4589] - Browser: Fix merging browser keys [#4685] - CLI: Fix encoding when exporting database [#3921] - SSH Agent: Improve reliability and underlying code [#3833, #4256, #4549, #4595] - FdoSecrets: Fix crash when editing settings before service is enabled [#4332]
|
Thank you! This had been a big pain for me on Linux. Now I can edit my databases on my SMB shares from all clients with ease. Thanks again! |
Type of change
Description and Context
This removes read-only mode.
The underlying motivation is #803 (and maybe #1937), where a qt bug triggers keepassxc to always open in read-only mode on some filesystems. I am also guessing that read-only mode is vestigial, and may be removed to work around the bug.
As I explained my comment on the issue, read-only mode seems to not be fully functional. I can't find any documentation on read-only mode or its place in the vision of multi-user / multi-client support in KeePassXC; my inference is that read-only mode is part of an old "office-style" mandatory explicit locking mechanism, and KeeShare is the new mechanism.
If I got the vision wrong, please let me know. I find it's often easiest to start a conversation with a PR.
Fixes #803
Testing strategy
I ran the existing unit tests and verified that I can open existing databases via gvfs and work with them normally, per #803.
Checklist:
-DWITH_ASAN=ON. [REQUIRED]